Issue 620: Close the fileChannels for read when they are idle by ArvinDevel · Pull Request #832 · apache/bookkeeper

ArvinDevel · 2017-12-12T04:05:46Z

Descriptions of the changes in this PR:

use guava cache to replace concurrentMap for logid2fileChannel, as guava cache has a eviction way, so we can use it to close the idle fileChannel

Master Issue: #620

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

If this PR is a BookKeeper Proposal (BP):

Make sure the PR title is formatted like:
<BP-#>: Description of bookkeeper proposal
e.g. BP-1: 64 bits ledger is support

Attach the master issue link in the description of this PR.

Attach the google doc link if the BP is written in Google Doc.

Otherwise:

Make sure the PR title is formatted like:
<Issue # or BOOKKEEPER-#>: Description of pull request
e.g. Issue 123: Description ...
e.g. BOOKKEEPER-1234: Description ...

Make sure tests pass via mvn clean apache-rat:check install findbugs:check.

Replace <Issue # or BOOKKEEPER-#> in the title with the actual Issue/JIRA number.

eolivelli

nice improvement, left some comments

eolivelli · 2017-12-12T12:26:14Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

    };

+    private final  CacheLoader<Long, FileChannel> loader = new CacheLoader<Long, FileChannel> () {
+        public FileChannel load(Long entryLogId) throws Exception {


maybe you can use lambda

I am not sure if you can use lambda for CacheLoader here, it is not an interface. CacheLoader is an abstract class

@eolivelli Just as @sijie said, can't use lambda due to it's an abstract class

eolivelli · 2017-12-12T12:26:18Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+    };
+    // close the file channel, when it was removed from cache
+    private final RemovalListener<Long, FileChannel> removalListener = new RemovalListener<Long, FileChannel>() {
+        public void onRemoval(RemovalNotification<Long, FileChannel> removal) {


maybe you can use lambda

eolivelli · 2017-12-12T12:26:42Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+        }
+    };
+
+    private final ExecutorService removeExecutor = Executors.newSingleThreadExecutor();


can we give a name to this thread ?

eolivelli · 2017-12-12T15:51:30Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

     */
-    private final ConcurrentMap<Long, FileChannel> logid2FileChannel = new ConcurrentHashMap<Long, FileChannel>();
+    private LoadingCache<Long, FileChannel> logid2FileChannel =  CacheBuilder.newBuilder()
+                .expireAfterAccess(1, TimeUnit.HOURS)


@sijie should be this value configurable ?
Maybe having a configuration value will let us write some test

+1 for making this configurable.

sijie

@ArvinDevel

I think there is one issue in this change, because the FileChannel in logid2FileChannel is shared by multiple threads in logid2Channel. it becomes problematic when FileChannel is evicted from logid2FileChannel but logid2Channel might still reference the underlying FileChannel.

so I would suggest:

having a reference count structure wrapping over FileChannel. E.g. ReferenceCountedFileChannel.
making logid2FileChannel still a concurrent map holding the reference counted file channels.

ConcurrentMap<Long, ReferenceCountedFileChannel> logid2FileChannel = new ConcurrentHashMap<Long, ReferenceCountedFileChannel>();

change logid2Channel to ThreadLocal<Cache<Long, BufferedReadChannel>> with concurrentLevel(1).
when a BufferedReadChannel is created, it gets the FileChannel from logid2FileChannel and increment the reference count of this file channel.
when a BufferedReadChannel is evicted or closed, decrement the reference count of this file channel. if a file channel is not referenced any more, close the file channel.

Also it would be good to add test cases for this change.

sijie · 2017-12-12T19:10:41Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+        } catch (ExecutionException e){
+            LOG.error("ExecutionException found in get fileChannel for log {} in logid2FileChannel cache", entryLogId);
+            // throw exception to avoid pass null to BufferedReadChannel
+            throw new IOException(e);


ExecutionException wrap the actual cause on loading the file channel. so you need to unwrap this.

if (e.getCause() instanceof IOException) { throw (IOException) e.getCause(); } else { throw new IOException("Encountered unknown exception on opening read channel for entry log " + entryLogId, e.getCause()); }

sijie · 2017-12-12T19:14:03Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

    };

+    private final  CacheLoader<Long, FileChannel> loader = new CacheLoader<Long, FileChannel> () {
+        public FileChannel load(Long entryLogId) throws Exception {


I am not sure if you can use lambda for CacheLoader here, it is not an interface. CacheLoader is an abstract class

sijie · 2017-12-12T19:15:08Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

        }
    };

+    private final  CacheLoader<Long, FileChannel> loader = new CacheLoader<Long, FileChannel> () {


I would suggest renaming this to a more meaningful name, like readonlyFileChannelLoader.

sijie · 2017-12-12T19:16:43Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+        }
+    };
+    // close the file channel, when it was removed from cache
+    private final RemovalListener<Long, FileChannel> removalListener = new RemovalListener<Long, FileChannel>() {


rename this to 'readonlyFileChannelRemovalListener'

sijie · 2017-12-12T19:17:36Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

     */
-    private final ConcurrentMap<Long, FileChannel> logid2FileChannel = new ConcurrentHashMap<Long, FileChannel>();
+    private LoadingCache<Long, FileChannel> logid2FileChannel =  CacheBuilder.newBuilder()
+                .expireAfterAccess(1, TimeUnit.HOURS)


+1 for making this configurable.

sijie · 2017-12-12T19:19:30Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-    private final ConcurrentMap<Long, FileChannel> logid2FileChannel = new ConcurrentHashMap<Long, FileChannel>();
+    private LoadingCache<Long, FileChannel> logid2FileChannel =  CacheBuilder.newBuilder()
+                .expireAfterAccess(1, TimeUnit.HOURS)
+                .removalListener(RemovalListeners.asynchronous(removalListener, removeExecutor))


I don't think you need to use asynchronous removal listener here. because this would somehow change the behavior of close and removeFromChannelsAndClose. I would suggest using removalListener here, so the behavior will be the same as what it was before when a filechannel is removed and closed.

sijie · 2017-12-13T20:30:20Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;

+


remove these 3 blank lines

sijie · 2017-12-13T20:33:19Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            return CacheBuilder.newBuilder().concurrencyLevel(1)
+                    .expireAfterAccess(expireReadChannelCacheInHour, TimeUnit.HOURS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get(removal.getKey()).release())


it might be good to check if the returned value is null or not.

ReferenceCountUtils.release(logid2FileChannel.get(removal.getKey());

I had not found this class in the project, and the AbstractReferenceCounted will check every release number to ensure that every release is valid and only after the refCnt == 0, the object will be deallocated.

You can use this one: https://netty.io/4.0/api/io/netty/util/ReferenceCountUtil.html

sijie · 2017-12-13T20:41:41Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-                LOG.warn("Exception while closing channel for log file:" + logId);
-            }
-        }
+        //remove the fileChannel from logId2Channel


I don't think you need to invalidate the local thread cache. The original behavior is to close the file channel directly, the local thread cache might be still referencing this file channel, but it is okay since the file channel is anyway closed.

I would suggest

add a method called forceCloseFileChannel() in ReferenceCountedFileChannel

deallocate() call this forceCloseFileChannel()

in here, you just remove it from logid2FileChannel and ``forceCloseFileChannel()` it.

ReferenceCountedFileChannel fileChannel = logid2FileChannel.remove(logId); if (null != fileChannel) { fileChannel.forceCloseFileChannel(); }

I think the new method is not necessary, just keep one deallocate is simple.

sijie · 2017-12-13T20:42:03Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

remove blank line

sijie · 2017-12-13T20:43:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-            newFc = oldFc;
-        }
+        FileChannel fc = new RandomAccessFile(file, "r").getChannel();
+        logid2FileChannel.put(entryLogId, new ReferenceCountedFileChannel(fc));


the logic should be similar as before. you need to use putIfAbsent for concurrent operations.

sijie · 2017-12-13T20:43:16Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-                fc.close();
+            //close corresponding fileChannel
+            for (ReferenceCountedFileChannel rfc : logid2FileChannel.values()) {
+                rfc.deallocate();


forceClose

sijie · 2017-12-13T20:44:03Z

@ArvinDevel left a few more comments on your latest changes. can you also add a test case for it?

ArvinDevel

there is a question, how to verify the fileChannel is closed? currently I checked the refCnt in test.

ArvinDevel · 2017-12-15T06:45:56Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            return CacheBuilder.newBuilder().concurrencyLevel(1)
+                    .expireAfterAccess(expireReadChannelCacheInHour, TimeUnit.HOURS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get(removal.getKey()).release())


I had not found this class in the project, and the AbstractReferenceCounted will check every release number to ensure that every release is valid and only after the refCnt == 0, the object will be deallocated.

ArvinDevel · 2017-12-15T06:51:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-                LOG.warn("Exception while closing channel for log file:" + logId);
-            }
-        }
+        //remove the fileChannel from logId2Channel


I think the new method is not necessary, just keep one deallocate is simple.

ArvinDevel · 2017-12-15T06:54:01Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-            newFc = oldFc;
-        }
+        FileChannel fc = new RandomAccessFile(file, "r").getChannel();
+        logid2FileChannel.put(entryLogId, new ReferenceCountedFileChannel(fc));


ArvinDevel · 2017-12-21T15:16:29Z

@eolivelli @sijie Any other suggestions?

sijie · 2017-12-27T07:49:36Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java

+        assertEquals(0, logid2FileChannel.get(2L).refCnt());
+        assertEquals(1, logid2FileChannel.get(3L).refCnt());
+        assertEquals(1, logid2FileChannel.get(4L).refCnt());
+//        assertNull(logid2FileChannel.get(2L).getFc());


remove this line if it is not needed

sijie · 2017-12-27T07:51:13Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

+     *          expire time.
+     * @return server configuration object.
+     */
+    public ServerConfiguration setExpireReadChannelCache(long millis) {


it is better to call out "time" and "ms" in the setting, like READ_CHANNEL_CACHE_EXPIRE_TIME_MS

sijie · 2017-12-27T07:53:29Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            return CacheBuilder.newBuilder().concurrencyLevel(1)
+                    .expireAfterAccess(expireReadChannelCacheInHour, TimeUnit.HOURS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get(removal.getKey()).release())


You can use this one: https://netty.io/4.0/api/io/netty/util/ReferenceCountUtil.html

sijie · 2017-12-27T07:59:26Z

there is a question, how to verify the fileChannel is closed? currently I checked the refCnt in test.

A tricky solution is to attempt to write data to a file channel. If a file channel is closed, the write will be thrown with ChannelClosedException

ArvinDevel · 2017-12-28T07:05:32Z

@sijie @eolivelli have fixed comments

sijie

@ArvinDevel one last comment about the configuration parameter.

sijie · 2017-12-28T10:07:11Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java

    protected static final String COMPACTION_RATE_BY_ENTRIES = "compactionRateByEntries";
    protected static final String COMPACTION_RATE_BY_BYTES = "compactionRateByBytes";
-    protected static final String EXPIRE_READ_CHANNEL_CACHE = "expireReadChannelCache";
+    protected static final String READ_CHANNEL_CACHE_EXPIRE_TIME_MS = "expireReadChannelCache";


you need to change the setting key "readChannelCacheExpireTimeMs"

sijie · 2017-12-28T10:08:01Z

@eolivelli please also review it since you are involved in this pull request before.

eolivelli

@ArvinDevel @sijie I left some other minor comments. We are on the right way

eolivelli · 2017-12-28T13:04:09Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

    };

+    // only used for test.
+    ThreadLocal<Cache<Long, BufferedReadChannel>> getLogid2Channel() {


@sijie I wonder if in the future we will drop rhis way of accessing internal state in fabour od something like powermock whitebox.

accessing internal state is okay if you are just testing the specific test case. I don't see a reason we will change it in future.

@ArvinDevel can you add @VisibleForTesting?

eolivelli · 2017-12-28T16:14:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+    private ConcurrentMap<Long, ReferenceCountedFileChannel>
+            logid2FileChannel = new ConcurrentHashMap<>();
+    // only for test.
+    ConcurrentMap<Long, ReferenceCountedFileChannel> getLogid2FileChannel() {


add VisibleForTesting

eolivelli · 2017-12-28T16:15:18Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-        return fc;
+        brc = new BufferedReadChannel(newFc, conf.getReadBufferBytes());
+        putInReadChannels(entryLogId, brc);
+        LOG.info("put readChannel: {}, corresponding to: {} ", brc, entryLogId);


drop debug ?

thanks,fixed

eolivelli · 2017-12-28T16:18:25Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java

+        long[][] positions = new long[numLogs][];
+        for (int i = 0; i < numLogs; i++) {
+            positions[i] = new long[numEntries];
+            EntryLogger logger = new EntryLogger(conf,


should we shutdown this EntryLogger ?

that would be good.

eolivelli · 2017-12-28T16:20:22Z

bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java

+        // the cache has readChannel for 2.log
+        assertNotNull(cacheThreadLocal.get().getIfPresent(2L));
+        // expire time
+        Thread.sleep(1000);


is there a way not to use a blind sleep ? but wait in a loop until a condition is met or a timeout fires ?

ExpireTime semantic is about a period, I think timeout is just like sleep.

@ArvinDevel : guava cache has a mock ticker that allows you to advance time to trigger expriation.

ArvinDevel · 2018-01-04T07:24:14Z

@sijie @eolivelli please review the latest changes.

eolivelli

+1

ivankelly

There's a race condition. We dealt with similar in #913 .

It shouldn't be so hard to fix here though, since the resources being cached are read only.

Why do we need a shared map of the file channels at all? The only issue with having multiple filechannels for the same files is that it increases the number of open files, but we could control that by setting the max entries to maxfd/num_read_threads. If we allow these dupes, then there's no need for reference counting and we can let the Cache deal with all loading and releasing.

ivankelly · 2018-01-05T10:33:16Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-            newFc = oldFc;
+            newFc = oldFc.fc;
+            // increment the refCnt
+            oldFc.retain();


It's possible that oldFc could have been released between getting it from logid2FileChannel and calling retain, so the channel returned could be closed.

you're right, I'll consider this later, thanks.

ivankelly · 2018-01-05T10:49:44Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+                    .expireAfterAccess(readChannelCacheExpireTimeMs, TimeUnit.MILLISECONDS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get(removal.getKey()).release())
+                    .build(readChannelLoader);


build(this::getChannelForLogId) should work here, i think, so you don't have to define the loader at all.

this loader is not an interface, so I can't use this functional style.

It's an abstract class with only one abstract method, so it should be usable.

@ivankelly I don't think it is worth on stucking here whether to use lambda or not. thoughts?

it's a minor thing. the important thing is that there are no races.

ivankelly · 2018-01-05T11:03:46Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+        @Override
+        protected void deallocate() {
+            try {
+                fc.close();


we close when refcount hits 0. When is this removed from the map though?

currently the ReferenceCountedFileChannel was not removed from the map, use guava cache to finish this would be better.

@ivankelly I have fixed your comment. I'm not sure whether it still has race condition, please has a review. If it still has race condition, I'll try use your design in #913. And the last resort can be removing hashMap and use multiple fileChannel directly.

I describe the design briefly: refCnt in AbstractReferenceCounted is volatile and dead is AtomicBoolean. Assume current refCnt is one and thread A is release()ing and thread B is retain()ing. If thread A is executed before thread B, as long as dead is changed, thread B will use the new fileChannel in case of using closed one.

sijie · 2018-01-17T04:34:15Z

@ArvinDevel did you have time to address @ivankelly 's comments?

ArvinDevel · 2018-01-17T06:46:14Z

I'll check the #913 and try to finish it in this week.

ivankelly

There's still races here :/ I'd suggest doing something very similar to the FileInfoBackingCache, as it's a very similar usecase. This should be simpler even, as there's no harm in having multiple copies of a file channel for a single file, as long as they are eventually closed.

ivankelly · 2018-01-30T15:51:48Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+                    .expireAfterAccess(readChannelCacheExpireTimeMs, TimeUnit.MILLISECONDS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get(removal.getKey()).release())
+                    .build(readChannelLoader);


It's an abstract class with only one abstract method, so it should be usable.

ivankelly · 2018-01-30T15:52:53Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-        Map<Long, BufferedReadChannel> threadMap = logid2Channel.get();
-        return threadMap.put(logId, bc);
+    public void putInReadChannels(long logId, BufferedReadChannel bc) {
+        Cache<Long, BufferedReadChannel> threadCahe = logid2Channel.get();


ivankelly · 2018-01-30T16:00:45Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-            newFc = oldFc;
+            // increment the refCnt
+            // double check to ensure the fileChannel is not closed due to refCnt down to 0.
+            if (oldFc.refCnt() > 0 && !oldFc.dead.get()) {


A cache on another thread could have released between L1176 and L1177, invalidating the oldFc object.

ivankelly · 2018-01-30T16:03:02Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

        FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-        FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
+        ReferenceCountedFileChannel oldFc =
+                logid2FileChannel.putIfAbsent(entryLogId, new ReferenceCountedFileChannel(newFc));


I don't see anywhere that entries are removed from logid2FileChannel.

sijie · 2018-01-31T21:58:23Z

@ArvinDevel can you address @ivankelly 's comments?

ArvinDevel · 2018-02-02T02:05:21Z

@sijie , I found @ivankelly 's FileInfoBackingCache design can address his comments, I'll adopt his elegant design.

ArvinDevel · 2018-02-05T09:05:00Z

@ivankelly thanks for your design and I adopted it, please review it when you have time.

ivankelly

There are still concurrency issues. This stuff needs a lot more unit tests also, to shake out any concurrency issues there may be.

ivankelly · 2018-02-06T14:47:02Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+
+            /**
+             * Attempt to retain the file info.
+             * When a client obtains a fileinfo from a container object,


this is the fileinfo comment. needs to be changed for this usage.

ivankelly · 2018-02-06T14:47:43Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+     * @see FileInfoBackingCache
     */
-    private final ConcurrentMap<Long, FileChannel> logid2FileChannel = new ConcurrentHashMap<Long, FileChannel>();
+    class FileChannelBackingCache {


Move into it's own file.

ivankelly · 2018-02-06T14:51:54Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

     */
-    private final ThreadLocal<Map<Long, BufferedReadChannel>> logid2Channel =
-            new ThreadLocal<Map<Long, BufferedReadChannel>>() {
+    private final ThreadLocal<Cache<Long, BufferedReadChannel>> logid2Channel =


you need to rename logid2Channel or logid2FileChannel. There's not semantic differences between the names, but they are very different. The similarity make the source very confusing to read.

may I suggest logId2ReadChannel and fileChannelBackingCache

ivankelly · 2018-02-06T14:55:48Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            return CacheBuilder.newBuilder().concurrencyLevel(1)
+                    .expireAfterAccess(readChannelCacheExpireTimeMs, TimeUnit.MILLISECONDS)
+                    //decrease the refCnt
+                    .removalListener(removal -> logid2FileChannel.get((Long) removal.getKey()).release())


The BufferedReadChannel should have a reference to the CachedFileChannel so you can do removal.getValue().release(). Looking up the value again is not guarantee that you'll get the same instance. It logically should give you it, but the logic is tangled.

I think that adding a readLock when getting value from FileChannelBackingCache has the same effect, and that has little change to BufferedReadChannel. I prefer to this one, which one is better in your opinion?

A lock would just tangle it up more.

You don't have to modify BufferedReadChannel. There's a couple of options.

You could create a specialization of BufferedReadChannel local to this class which has the CachedFileChannel as a member and use that in the tls cache.

You could create a container class which references both the bufferedReadChannel and the CachedFileChannel and use that in the TLS cache.

I think 1 is the better option, as you'll only have to change a few signatures.

ivankelly · 2018-02-06T14:58:29Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+        CachedFileChannel loadFileChannel(long logId) throws IOException {
+            CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+            if (cachedFileChannel != null) {
+                boolean retained = cachedFileChannel.tryRetain();


if cachedFileChannel existed in fileChannels, then it's quite possible that another reference to it exists from a previous call to loadFileChannel. If another reference exists, then another the holder of the other reference could release it, so there's no guarantee that tryRetain would return true.

um, that's right. The ReadWriteLock is necessary, I'll refactor again. I thought that lock is used to guarantee FileInfo's file open/write operations before now.

Yes, you'll need some locking here. Again, FileInfoBackingCache is an example to use.

ivankelly · 2018-02-06T14:59:52Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            // it would be better to open using read mode
+            FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
+            cachedFileChannel = new CachedFileChannel(logId, newFc);
+            fileChannels.put(logId, cachedFileChannel);


What if another thread has added a cachedFileChannel for the same logId since L385?

ivankelly · 2018-02-06T15:00:50Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+                } finally {
+                    IOUtils.close(LOG, fc.fc);
+                }
+                fileChannels.remove(logId);


No guarantee that the CachedFileChannel being removed from fileChannels is actually the one referenced by fc.

I'll use lock to avoid multi write/delete operations to the fileChannels.

it shouldn't need locking. there's a remove method that takes the key and the value, and only removes if both match.

okay, then I wonder why you still use writeLock in releaseFileInfo method? see code. Can we reduce lock in there? or the lock is a must when change the state of concurrentHashMap?

The write lock is needed because marking a fileinfo as dead, flushing its contents to disk and removing it from the map needs to be an atomic operation. If the lock wasn't there, then we could mark as dead, then before removing from map another thread could call loadFileInfo() and end up returning a dead fileinfo (tryRetain would fail). this would put the caller into a tight loop. We could, i think, remove the lock entirely from this class, but I'm very reluctant to mess with it, since this stuff is hard to get right (and it's often very hard to tell that you got it wrong).

ivankelly · 2018-02-06T15:11:16Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+            } while (!cachedFileChannel.tryRetain());
+        } finally {
+            if (null != cachedFileChannel) {
+                cachedFileChannel.release();


?
You're releasing the reference straight away after getting it. By the time cachedFileChannel is used on L1278, the reference could easily be dead.

loadFileChannel and tryRetain both add one reference count, so releasing one is necessary.

the refcount for loadFileChannel belongs to the cache.
the refcount for tryRetain belongs to the created BufferedReadChannel.

If the CachedFileChannel is invalidated from the cache, you don't want it to be closed immediately, you want it to be closed when the BufferedReadChannel releases it. Similarly, if the BufferedReadChannel releases it, and it is still in the cache, you don't want to close the file channel, as the cache may hand it out to another caller. So you need both of them to hold a refcount.

asfgit · 2018-02-16T10:23:55Z

Can one of the admins verify this patch?

sijie

I have lost the track why we are switching to FileChannelBackingCache here. As I can see, there are a couple of issue inFileChannelBackingCache itself. The problem might also exist in FileInfoBackingCache.

I think the original implementation is much simpler approach. the difference between this problem and file info cache is, file info cache needs to handle both write and read, while the problem here just need to handle read only channels. They don't modify persistence. We should handle it in a much simpler way than FileInfoCache. I think we are making things complicated here. But I need to go through the whole history to understand why do you guys want to do that. that's a separate topic.

For the approach we are using here, I would suggest:

can you guys answer my question regarding these XYZBackingCache things? I see quite bunch of issues there: e.g. not well performat (locking while doing I/O), race conditions.
If we are going to take the BackingCache approach, let's do a refactor to make it a generic class. Duplicating the logic isn't the right approach.

sijie · 2018-03-02T19:40:24Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+                    CacheBuilder.newBuilder().concurrencyLevel(1)
+                    .expireAfterAccess(readChannelCacheExpireTimeMs, TimeUnit.MILLISECONDS)
+                    //decrease the refCnt
+                    .removalListener(( RemovalListener<Long, EntryLogBufferedReadChannel>) removal


nit:

it is a bit hard to read. can you write it in a better format? also remove space before RemovalListener.

.removal((RemovalListener<Long, EntryLogBufferedReadChannel>) notification -> removal.getValue().release() ) .ticker(getTicker()) .build();

sijie · 2018-03-02T19:40:38Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

-    public BufferedReadChannel getFromChannels(long logId) {
-        return logid2Channel.get().get(logId);
-    }
+    FileChannelBackingCache fileChannelBackingCache = new FileChannelBackingCache(this::findFile);


nit: make it final

sijie · 2018-03-02T19:41:44Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+
+            // entrySize does not include the ledgerId
+            if (entrySize > maxSaneEntrySize) {
+                LOG.warn("Sanity check failed for entry size of " + entrySize + " at location " + pos + " in "


nit:

it can be written with '{}'

sijie · 2018-03-02T19:46:10Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+    private EntryLogBufferedReadChannel getChannelForLogId(long entryLogId) throws IOException {
+        try {
+            EntryLogBufferedReadChannel brc;
+            Callable<EntryLogBufferedReadChannel> loader = () -> {


why do we create loader everytime, this is generating a lot of garbages on jvm? I remember the first time I reviewed this, I don't think see loader here...

why can't we use LoadingCache instead?

A better improvement, I have fixed it.

sijie · 2018-03-02T19:47:04Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java

+                    LOG.error("Dead fileChannel({}) forced out of cache."
+                            + "It must have been double-released somewhere.", brc.cachedFileChannel);
+                    }
+                brc = null;


need 4 more spaces indent.

sijie · 2018-03-02T19:50:24Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+        }
+    }
+
+    class CachedFileChannel {


can be static

Because it uses FileChannelBackingCache's non-static method releaseFileChannel, so it can't be static

sijie · 2018-03-02T19:57:18Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+            CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+            if (cachedFileChannel != null) {
+                boolean retained = cachedFileChannel.tryRetain();
+                checkArgument(retained);


who is going to catch IllegalArgumentException?

if fail to retain, we should throw IOException not a runtime exception, no?

sijie · 2018-03-02T19:58:32Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+
+        lock.writeLock().lock();
+        try {
+            File file = fileLoader.load(logId);


why do we need to lock while loading a file?

don't we just need to lock when putting the channel back?

If this is borrowed from FileInfoBackingCache, same question is applied there.

This guarantees the refCnt's correctness.
Imagine the scenario: thread A and thread B both want to load the same fileChannel without lock:
1.A and B both run to the line after created fileChannel respectively;
2.A execute:
fileChannels.put(logId, cachedFileChannel); boolean retained = cachedFileChannel.tryRetain();;
3.A was switched and B execute these line again;
Then the FileChannelBacking cache will hold the last fileChannel with one refCnt, but the refCnt should be two actually.

@sijie for fileinfobackingcache, we need to load under a lock to have mutal exclusion with another thread that is flushing. That isn't the case here, since there's no harm in having two channels to the same file open.

So this can probably be changed to remove the locking completely. When you load a new entry, do a putIfAbsent, and if it fails, clean up what you just opened. Alternatively, computeIfAbsent could be used.

sijie · 2018-03-02T20:03:44Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+            // it would be better to open using read mode
+            FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
+            CachedFileChannel cachedFileChannel = new CachedFileChannel(logId, newFc);
+            fileChannels.put(logId, cachedFileChannel);


you need to check the return value. there can be race condition that a file channel is added after get.

Yes, just like mentioned above, the writeLock can guarantee it.

sijie · 2018-03-02T20:04:19Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+     * @param cachedFileChannel
+     */
+    private void releaseFileChannel(long logId, CachedFileChannel cachedFileChannel) {
+        lock.writeLock().lock();


same here, I am not sure why we are closing a file channel under a write lock.

so the question is what does lock is actually locking.

The lock can guarantee the fileChannel being loading not closed by another thread.
Below is a race condition.
Thread A holds one refCnt for the specific fileChannel and release it, so the refCnt is 0 and being closed.
Thread B load the fileChannel with ReadLock, so it can't continue if thread A call releaseFileChannel firstly as the Thread A holds writeLock. This case is fine.
If thread B add refCnt before thread A get writeLock, then releaseFileChannel 's logic guarantee the fileChannel was not closed by check markDead() under writeLock. So it still works fine under this case.

Only the removal from the map needs to be under the lock, to prevent another thread from handing out file channel being closed. Once the channel has been removed from the map, it can be closed outside of the lock. This is different than with fileinfo, because fileinfo needs to flush it's contents out to disk, ensuring noone else reads them before they are fully flushed.

@ivankelly I think the markDead() needs to be under the lock to achieve preventing another thread from handing out file channel being closed. Close operation can be move out completely.

markDead doesn't need to be under a lock. The caller should call tryRetain() after receiving, which will either prevent markDead from having an effect, if it is called before it.

Then we need some do while check in loadFileChannel.
I update again.

…ava cache has a eviction way, so we can use it to close the idle fileChannel

…ReferenceCountedFileChannel

ivankelly · 2018-03-05T10:01:07Z

I think the original implementation is much simpler approach.

The original implementation had race conditions, where dead file channels would be handed out, which is why I suggested Arvin take a look at the refcounting stuff from the fileinfobackingcache, which dealt with a similar problem. The locking from the fileinfobackingcache is probably not needed though.

ivankelly · 2018-03-14T15:57:28Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+                    FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
+                    cfc = new CachedFileChannel(logFileId, newFc);
+                } catch (IOException ioe){
+                    throw new UncheckedIOException(ioe);


This will end up doing throwing a RuntimeException. Rather than using a call to computeIfAbsent(), do:

CachedFileChannel cachedFileChannel = null; do { CachedFileChannel c = fileChannels.get(logId); if (c != null) { if (c.tryRetain()) { cachedFileChannel = c; } else { // this isn't strictly necessary, but it's good defensively to avoid infinite loop fileChannels.remove(logId, c); } } else { c = // the construction stuff CachedFileChannel existing = fileChannels.putIfAbsent(logId, c); if (existing != null) { // cleanup c if (existing.tryRetain()) { cachedFileChannel = existing; } else { fileChannels.remove(logId, existing); } } } } while (cachedFileChannel == null);

ivankelly · 2018-03-14T15:58:01Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

@@ -52,36 +53,33 @@ class FileChannelBackingCache {
    final ConcurrentHashMap<Long, CachedFileChannel> fileChannels = new ConcurrentHashMap<>();

    CachedFileChannel loadFileChannel(long logId) throws IOException {


lock field should be no longer used, so it can be removed from the class.

ivankelly · 2018-03-14T15:59:46Z

bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java

+            fileChannels.remove(logId, cachedFileChannel);
+            // close corresponding fileChannel
+            try {
+                cachedFileChannel.fileChannel.close();


it would be better is cachedFileChannel had a close method rather than directly accessing members.

Descriptions of the changes in this PR: There are a couple of issues noticed in FileInfoBackingCache: 1) There is a race condition in loadFileInfo between get-check and put. If concurrent loading happens, there might be a FileInfo loaded into the map after get-check. This can cause incorrect reference count on FileInfo. 2) FileLoader is doing I/O operation which happens under a giant write lock. 3) assert is typically not recommended since it is disabled at production runtime typically. *Changes* - Check whether fileinfo exists or not after getting write lock and before put - Move any I/O operations out of write lock - release the new FileInfo if concurrent puts happen - remove the usage of assert Beside that, switch to use ConcurrentLongHashMap to avoid boxing and unboxing. Related Issues: #913 #832 Author: Sijie Guo <sijie@apache.org> Reviewers: Ivan Kelly <ivank@apache.org> This closes #1284 from sijie/improve_fileinfo_backing_cache

jvrao · 2019-05-29T05:00:41Z

retest

eolivelli · 2020-05-17T12:16:07Z

closing for inactivity. feel free to reopen

eolivelli requested changes Dec 12, 2017

View reviewed changes

sijie reviewed Dec 12, 2017

View reviewed changes

sijie reviewed Dec 13, 2017

View reviewed changes

ArvinDevel commented Dec 15, 2017

View reviewed changes

ArvinDevel force-pushed the issue620 branch from cfa2fe1 to 5d112e7 Compare December 15, 2017 14:44

sijie reviewed Dec 27, 2017

View reviewed changes

sijie reviewed Dec 28, 2017

View reviewed changes

eolivelli reviewed Dec 28, 2017

View reviewed changes

ArvinDevel force-pushed the issue620 branch from e39485e to 53173b3 Compare January 4, 2018 07:22

eolivelli approved these changes Jan 4, 2018

View reviewed changes

ivankelly requested changes Jan 5, 2018

View reviewed changes

ivankelly reviewed Jan 5, 2018

View reviewed changes

ivankelly assigned ArvinDevel Jan 5, 2018

ArvinDevel force-pushed the issue620 branch from 53173b3 to ca1d87e Compare January 24, 2018 14:43

ivankelly requested changes Jan 30, 2018

View reviewed changes

ArvinDevel force-pushed the issue620 branch from ca1d87e to 0b724a6 Compare February 4, 2018 02:13

ivankelly requested changes Feb 6, 2018

View reviewed changes

ArvinDevel force-pushed the issue620 branch from 3d214f9 to 803b591 Compare February 11, 2018 02:39

sijie reviewed Mar 2, 2018

View reviewed changes

ArvinDevel added 19 commits March 5, 2018 17:09

use guava cache to replace concurrentMap for logid2fileChannel, as gu…

b0ac293

…ava cache has a eviction way, so we can use it to close the idle fileChannel

refactor to use guava cache to replace Map for logid2Channel and add …

3868617

…ReferenceCountedFileChannel

deallocate the removed fileChannel

b8031ff

add test case for file channel cache

a3dfe51

fix warning

ee6ea0c

fix comments

143d425

improve nits

4b4a8e8

improve cache test logic by using ticker in guava

a2e8c9d

add more check to guarantee the using fileChannel not closed

098327f

refactor to avoid race conditions

03693fe

use double check when get fileChannel from backingCache

9fda119

refactor impl

71e0cc3

refactor test of FileChannelBackingCache

4616c4e

fix nits

02c0662

fix nits

b5c09e7

use try-with-resources statements

b931c11

add a global cache containers to help invalidating readChannel

95081a7

add readChannelCacheExpireTimeMs to yaml file

b5f88eb

using LoadingCache to replace Cache to avoid gc

49667b6

ArvinDevel force-pushed the issue620 branch from c30de04 to 49667b6 Compare March 5, 2018 09:10

ArvinDevel added 2 commits March 6, 2018 10:31

reduce the usage of lock

8b5f941

reduce the usage of lock again

ea934b3

ivankelly reviewed Mar 14, 2018

View reviewed changes

sijie mentioned this pull request Mar 21, 2018

Improve FileInfoBackingCache #1284

Closed

eolivelli closed this May 17, 2020

		@@ -52,36 +53,33 @@ class FileChannelBackingCache {
		final ConcurrentHashMap<Long, CachedFileChannel> fileChannels = new ConcurrentHashMap<>();

		CachedFileChannel loadFileChannel(long logId) throws IOException {

Comments

Conversation

ArvinDevel commented Dec 12, 2017 • edited by sijie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eolivelli left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented Dec 13, 2017

Uh oh!

ArvinDevel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArvinDevel commented Dec 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sijie commented Dec 27, 2017

Uh oh!

ArvinDevel commented Dec 28, 2017

Uh oh!

ArvinDevel commented Dec 12, 2017 •

edited by sijie

Loading