[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172843523
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+} finally {
+lock.writeLock().unlock();
+}
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param cachedFileChannel
+ */
+private void releaseFileChannel(long logId, CachedFileChannel 
cachedFileChannel) {
+lock.writeLock().lock();
 
 Review comment:
   Then we need some do while check in loadFileChannel. 
   I update again.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172388151
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+} finally {
+lock.writeLock().unlock();
+}
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param cachedFileChannel
+ */
+private void releaseFileChannel(long logId, CachedFileChannel 
cachedFileChannel) {
+lock.writeLock().lock();
 
 Review comment:
   @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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172122421
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+} finally {
+lock.writeLock().unlock();
+}
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param cachedFileChannel
+ */
+private void releaseFileChannel(long logId, CachedFileChannel 
cachedFileChannel) {
+lock.writeLock().lock();
 
 Review comment:
   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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172122411
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1094,25 +1104,40 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 }
 }
 
-private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
-}
-File file = findFile(entryLogId);
-// get channel is used to open an existing entry log file
-// it would be better to open using read mode
-FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
-if (null != oldFc) {
-newFc.close();
-newFc = oldFc;
-}
-// We set the position of the write buffer of this buffered channel to 
Long.MAX_VALUE
-// so that there are no overlaps with the write buffer while reading
-fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes());
-putInReadChannels(entryLogId, fc);
-return fc;
+/**
+ * Add one refCnt for BufferedReadChannel and return it, caller need to 
subtract one refCnt.
+ * @param entryLogId
+ * @return
+ * @throws IOException
+ */
+private EntryLogBufferedReadChannel getChannelForLogId(long entryLogId) 
throws IOException {
+try {
+EntryLogBufferedReadChannel brc;
+Callable loader = () -> {
 
 Review comment:
   A better improvement, I have fixed it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172119279
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
+// get channel is used to open an existing entry log file
+// 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);
 
 Review comment:
   Yes, just like mentioned above, the writeLock can guarantee it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172119053
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
 
 Review comment:
   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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r172115608
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileChannelBackingCache.java
 ##
 @@ -0,0 +1,190 @@
+/*
+ *
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *
+ */
+
+package org.apache.bookkeeper.bookie;
+
+import static com.google.common.base.Preconditions.checkArgument;
+import com.google.common.annotations.VisibleForTesting;
+import java.io.File;
+import java.io.IOException;
+import java.io.RandomAccessFile;
+import java.nio.channels.FileChannel;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.concurrent.locks.ReentrantReadWriteLock;
+import org.apache.bookkeeper.util.IOUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of FileInfoBackingCache.
+ * @see FileInfoBackingCache
+ */
+class FileChannelBackingCache {
+private static final Logger LOG = 
LoggerFactory.getLogger(FileChannelBackingCache.class);
+static final int DEAD_REF = -0xdead;
+final ReentrantReadWriteLock lock = new ReentrantReadWriteLock();
+
+final FileLoader fileLoader;
+
+FileChannelBackingCache(FileLoader fileLoader) {
+this.fileLoader = fileLoader;
+}
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+lock.readLock().lock();
+try {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+}
+} finally {
+lock.readLock().unlock();
+}
+
+lock.writeLock().lock();
+try {
+File file = fileLoader.load(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+checkArgument(retained);
+return cachedFileChannel;
+} finally {
+lock.writeLock().unlock();
+}
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param cachedFileChannel
+ */
+private void releaseFileChannel(long logId, CachedFileChannel 
cachedFileChannel) {
+lock.writeLock().lock();
+try {
+if (cachedFileChannel.markDead()) {
+try {
+cachedFileChannel.fileChannel.close();
+} catch (IOException e) {
+LOG.warn("Exception occurred in 
ReferenceCountedFileChannel"
++ " while closing channel for log file: {}", 
cachedFileChannel);
+} finally {
+IOUtils.close(LOG, cachedFileChannel.fileChannel);
+}
+// to guarantee the removed cachedFileChannel is what we want 
to remove.
+fileChannels.remove(logId, cachedFileChannel);
+}
+} finally {
+lock.writeLock().unlock();
+}
+}
+
+@VisibleForTesting
+CachedFileChannel get(Long logId) {
+lock.readLock().lock();
+try {
+return fileChannels.get(logId);
+} finally {
+lock.readLock().unlock();
+}
+}
+
+class CachedFileChannel {
 
 Review comment:
   Because it uses FileChannelBackingCache's non-static method 
`releaseFileChannel`, so it can't be static

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171750928
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -703,7 +716,7 @@ void stop() {
  *  Entry Log File Id
  */
 protected boolean removeEntryLog(long entryLogId) {
-removeFromChannelsAndClose(entryLogId);
+fileChannelBackingCache.removeFromChannelsAndClose(entryLogId);
 
 Review comment:
   After add a global readChannel concurrent list, we can let invalidating 
cache to trigger close operation. So is it still a need to keep the 
removeFromChannelsAndClose()? I guess it's not anymore.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171535612
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +350,47 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> 
logid2ReadChannel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> ((EntryLogBufferedReadChannel) 
removal.getValue()).release())
 
 Review comment:
   I found that the cast is must because the removal's type is not declared.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171487369
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java
 ##
 @@ -391,4 +489,57 @@ public void testGetEntryLogsSet() throws Exception {
 
 assertEquals(Sets.newHashSet(0L, 1L, 2L, 3L), 
logger.getEntryLogsSet());
 }
+
+/**
+ * Fake Ticker to advance ticker as expected.
+ */
+class FakeTicker extends Ticker {
 
 Review comment:
   There exists indeed. Now I import the guava test-lib dependency to solve 
this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171483174
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1361,25 +1401,20 @@ public void shutdown() {
 // since logChannel is buffered channel, do flush when shutting down
 LOG.info("Stopping EntryLogger");
 try {
+//close corresponding fileChannels for read
+fileChannelBackingCache.closeAllFileChannels();
 
 Review comment:
   If we want to invalidate the threadLocal cache, then we has 2 ways to 
implement which was discussed in 
here(https://stackoverflow.com/questions/2795447/is-there-no-way-to-iterate-over-or-copy-all-the-values-of-a-java-threadlocal).
  I think invalidating method is complicated than simply closing in 
fileChannelBackingCache.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171481508
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1174,24 +1208,19 @@ private File findFile(long logId) throws 
FileNotFoundException {
  * @throws IOException
  */
 public void scanEntryLog(long entryLogId, EntryLogScanner scanner) throws 
IOException {
-// Buffer where to read the entrySize (4 bytes) and the ledgerId (8 
bytes)
-ByteBuf headerBuffer = Unpooled.buffer(4 + 8);
-BufferedReadChannel bc;
-// Get the BufferedChannel for the current entry log file
+EntryLogBufferedReadChannel bc = null;
 
 Review comment:
   I just move previous exception handling code's location. I think keeping 
original's exception handling logic is necessary.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r171480143
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1090,28 +1106,46 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 return new Header(headerVersion, ledgersMapOffset, ledgersCount);
 } finally {
 headers.release();
+if (bc != null) {
+bc.release();
+}
 }
 }
 
-private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
-}
-File file = findFile(entryLogId);
-// get channel is used to open an existing entry log file
-// it would be better to open using read mode
-FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
-if (null != oldFc) {
-newFc.close();
-newFc = oldFc;
-}
-// We set the position of the write buffer of this buffered channel to 
Long.MAX_VALUE
-// so that there are no overlaps with the write buffer while reading
-fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes());
-putInReadChannels(entryLogId, fc);
-return fc;
+/**
+ * Add one refCnt for BufferedReadChannel and return it, caller need to 
subtract one refCnt.
+ * @param entryLogId
+ * @return
+ * @throws IOException
+ */
+private EntryLogBufferedReadChannel getChannelForLogId(long entryLogId) 
throws IOException {
+try {
+EntryLogBufferedReadChannel brc;
+Callable loader = () -> {
+CachedFileChannel cachedFileChannel = 
fileChannelBackingCache.loadFileChannel(entryLogId);
+// We set the position of the write buffer of this buffered 
channel to Long.MAX_VALUE
+// so that there are no overlaps with the write buffer while 
reading
+return new EntryLogBufferedReadChannel(cachedFileChannel, 
conf.getReadBufferBytes());
+};
+do {
+// Put the logId, bc pair in the cache responsible for the 
current thread.
+brc = getThreadLocalCacheForReadChannel().get(entryLogId, 
loader);
+if (!brc.cachedFileChannel.tryRetain()) {
 
 Review comment:
   I think your method is not suitable because the refCnt of 
BufferedReadChannel to fileChannel will be two when it was loaded first time, 
in this situation, we don't need tryRetain() again. But when the 
BufferedReadChannel is already in the cache, we definitely need to tryRetain() 
to add refCnt.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r167393265
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +333,216 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> logid2FileChannel.get((Long) 
removal.getKey()).release())
+.build(readChannelLoader);
+}
+};
+
+@VisibleForTesting
+long getReadChannelCacheExpireTimeMs() {
+return readChannelCacheExpireTimeMs;
+}
+
+@VisibleForTesting
+CacheLoader getReadChannelLoader() {
+return readChannelLoader;
+}
+
+private final  CacheLoader readChannelLoader =
+new CacheLoader () {
+public BufferedReadChannel load(Long entryLogId) throws Exception {
+
+return getChannelForLogId(entryLogId);
+
 }
 };
 
 /**
- * Each thread local buffered read channel can share the same file handle 
because reads are not relative
- * and don't cause a change in the channel's position. We use this map to 
store the file channels. Each
- * file channel is mapped to a log id which represents an open log file.
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of 
FileInfoBackingCache.
+ * @see FileInfoBackingCache
  */
-private final ConcurrentMap logid2FileChannel = new 
ConcurrentHashMap();
+class FileChannelBackingCache {
+static final int DEAD_REF = -0xdead;
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+assert(retained);
+return cachedFileChannel;
+}
+File file = findFile(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+assert(retained);
+return cachedFileChannel;
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param fc
+ */
+private void releaseFileChannel(long logId, CachedFileChannel fc) {
+if (fc.markDead()) {
+try {
+fc.fc.close();
+} catch (IOException e) {
+LOG.warn("Exception occurred in 
ReferenceCountedFileChannel"
++ " while closing channel for log file: {}", fc);
+} finally {
+IOUtils.close(LOG, fc.fc);
+}
+fileChannels.remove(logId);
 
 Review comment:
   okay, then I wonder why you still use `writeLock` in `releaseFileInfo` 

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r167209456
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +333,216 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> logid2FileChannel.get((Long) 
removal.getKey()).release())
 
 Review comment:
   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?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r167131218
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +333,216 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> logid2FileChannel.get((Long) 
removal.getKey()).release())
+.build(readChannelLoader);
+}
+};
+
+@VisibleForTesting
+long getReadChannelCacheExpireTimeMs() {
+return readChannelCacheExpireTimeMs;
+}
+
+@VisibleForTesting
+CacheLoader getReadChannelLoader() {
+return readChannelLoader;
+}
+
+private final  CacheLoader readChannelLoader =
+new CacheLoader () {
+public BufferedReadChannel load(Long entryLogId) throws Exception {
+
+return getChannelForLogId(entryLogId);
+
 }
 };
 
 /**
- * Each thread local buffered read channel can share the same file handle 
because reads are not relative
- * and don't cause a change in the channel's position. We use this map to 
store the file channels. Each
- * file channel is mapped to a log id which represents an open log file.
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of 
FileInfoBackingCache.
+ * @see FileInfoBackingCache
  */
-private final ConcurrentMap logid2FileChannel = new 
ConcurrentHashMap();
+class FileChannelBackingCache {
+static final int DEAD_REF = -0xdead;
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
+assert(retained);
+return cachedFileChannel;
+}
+File file = findFile(logId);
+// get channel is used to open an existing entry log file
+// 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);
+boolean retained = cachedFileChannel.tryRetain();
+assert(retained);
+return cachedFileChannel;
+}
+
+/**
+ * close FileChannel and remove from cache when possible.
+ * @param logId
+ * @param fc
+ */
+private void releaseFileChannel(long logId, CachedFileChannel fc) {
+if (fc.markDead()) {
+try {
+fc.fc.close();
+} catch (IOException e) {
+LOG.warn("Exception occurred in 
ReferenceCountedFileChannel"
++ " while closing channel for log file: {}", fc);
+} finally {
+IOUtils.close(LOG, fc.fc);
+}
+fileChannels.remove(logId);
 
 Review comment:
   I'll use lock to avoid multi write/delete operations to the 

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r167131090
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +333,216 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> logid2FileChannel.get((Long) 
removal.getKey()).release())
+.build(readChannelLoader);
+}
+};
+
+@VisibleForTesting
+long getReadChannelCacheExpireTimeMs() {
+return readChannelCacheExpireTimeMs;
+}
+
+@VisibleForTesting
+CacheLoader getReadChannelLoader() {
+return readChannelLoader;
+}
+
+private final  CacheLoader readChannelLoader =
+new CacheLoader () {
+public BufferedReadChannel load(Long entryLogId) throws Exception {
+
+return getChannelForLogId(entryLogId);
+
 }
 };
 
 /**
- * Each thread local buffered read channel can share the same file handle 
because reads are not relative
- * and don't cause a change in the channel's position. We use this map to 
store the file channels. Each
- * file channel is mapped to a log id which represents an open log file.
+ * FileChannelBackingCache used to cache RefCntFileChannels for read.
+ * In order to avoid get released file, adopt design of 
FileInfoBackingCache.
+ * @see FileInfoBackingCache
  */
-private final ConcurrentMap logid2FileChannel = new 
ConcurrentHashMap();
+class FileChannelBackingCache {
+static final int DEAD_REF = -0xdead;
+
+final ConcurrentHashMap fileChannels = new 
ConcurrentHashMap<>();
+
+CachedFileChannel loadFileChannel(long logId) throws IOException {
+CachedFileChannel cachedFileChannel = fileChannels.get(logId);
+if (cachedFileChannel != null) {
+boolean retained = cachedFileChannel.tryRetain();
 
 Review comment:
   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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r167127930
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1094,24 +1259,25 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 }
 
 private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
-}
-File file = findFile(entryLogId);
-// get channel is used to open an existing entry log file
-// it would be better to open using read mode
-FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
-if (null != oldFc) {
-newFc.close();
-newFc = oldFc;
+BufferedReadChannel brc = getFromChannels(entryLogId);
+if (brc != null) {
+return brc;
+}
+FileChannelBackingCache.CachedFileChannel cachedFileChannel = null;
+try {
+do {
+cachedFileChannel = 
logid2FileChannel.loadFileChannel(entryLogId);
+} while (!cachedFileChannel.tryRetain());
+} finally {
+if (null != cachedFileChannel) {
+cachedFileChannel.release();
 
 Review comment:
   `loadFileChannel` and `tryRetain` both add one reference count, so releasing 
one is necessary.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r163572115
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +334,115 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> 
logid2FileChannel.get(removal.getKey()).release())
+.build(readChannelLoader);
+}
+};
+
+@VisibleForTesting
+long getReadChannelCacheExpireTimeMs() {
+return readChannelCacheExpireTimeMs;
+}
+
+@VisibleForTesting
+CacheLoader getReadChannelLoader() {
+return readChannelLoader;
+}
+
+private final  CacheLoader readChannelLoader =
+new CacheLoader () {
+public BufferedReadChannel load(Long entryLogId) throws Exception {
+
+return getChannelForLogId(entryLogId);
+
 }
 };
 
+static class ReferenceCountedFileChannel extends AbstractReferenceCounted {
+private final FileChannel fc;
+
+public ReferenceCountedFileChannel(FileChannel fileChannel) {
+this.fc = fileChannel;
+}
+
+@VisibleForTesting
+FileChannel getFileChannel(){
+return fc;
+}
+
+@Override
+public ReferenceCounted touch(Object hint) {
+return this;
+}
+
+// when the refCnt decreased to 0 or force deallocate
+@Override
+protected void deallocate() {
+try {
+fc.close();
 
 Review comment:
   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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r163568340
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +334,115 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> 
logid2FileChannel.get(removal.getKey()).release())
+.build(readChannelLoader);
 
 Review comment:
   this loader is not an interface, so I can't use this functional style.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r160021944
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1091,24 +1156,29 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 }
 
 private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
+BufferedReadChannel brc = getFromChannels(entryLogId);
+if (brc != null) {
+return brc;
 }
+
 File file = findFile(entryLogId);
 // get channel is used to open an existing entry log file
 // it would be better to open using read mode
 FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
+ReferenceCountedFileChannel oldFc =
+logid2FileChannel.putIfAbsent(entryLogId, new 
ReferenceCountedFileChannel(newFc));
 if (null != oldFc) {
 newFc.close();
-newFc = oldFc;
+newFc = oldFc.fc;
+// increment the refCnt
+oldFc.retain();
 
 Review comment:
   you're right, I'll consider this later, thanks.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r160021872
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -330,54 +334,115 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(readChannelCacheExpireTimeMs, 
TimeUnit.MILLISECONDS)
+//decrease the refCnt
+.removalListener(removal -> 
logid2FileChannel.get(removal.getKey()).release())
+.build(readChannelLoader);
+}
+};
+
+@VisibleForTesting
+long getReadChannelCacheExpireTimeMs() {
+return readChannelCacheExpireTimeMs;
+}
+
+@VisibleForTesting
+CacheLoader getReadChannelLoader() {
+return readChannelLoader;
+}
+
+private final  CacheLoader readChannelLoader =
+new CacheLoader () {
+public BufferedReadChannel load(Long entryLogId) throws Exception {
+
+return getChannelForLogId(entryLogId);
+
 }
 };
 
+static class ReferenceCountedFileChannel extends AbstractReferenceCounted {
+private final FileChannel fc;
+
+public ReferenceCountedFileChannel(FileChannel fileChannel) {
+this.fc = fileChannel;
+}
+
+@VisibleForTesting
+FileChannel getFileChannel(){
+return fc;
+}
+
+@Override
+public ReferenceCounted touch(Object hint) {
+return this;
+}
+
+// when the refCnt decreased to 0 or force deallocate
+@Override
+protected void deallocate() {
+try {
+fc.close();
 
 Review comment:
   currently the ReferenceCountedFileChannel was not removed from the map, use 
guava cache to finish this would be better.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r158970913
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java
 ##
 @@ -315,6 +316,104 @@ public void testRecoverFromLedgersMapOnV0EntryLog() 
throws Exception {
 assertEquals(120, meta.getRemainingSize());
 }
 
+/**
+ * Test Cache for logid2Channel and concurrentMap for logid2FileChannel 
work correctly.
+ * Note that, when an entryLogger is initialized, the entry log id will 
increase one.
+ * when the preallocation is enabled, a new entrylogger will cost 2 logId.
+ */
+@Test
+public void testCacheInEntryLog() throws Exception {
+File tmpDir = createTempDir("bkTest", ".dir");
+File curDir = Bookie.getCurrentDirectory(tmpDir);
+Bookie.checkDirectoryStructure(curDir);
+
+int gcWaitTime = 1000;
+ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
+conf.setGcWaitTime(gcWaitTime);
+conf.setLedgerDirNames(new String[] {tmpDir.toString()});
+//since last access, expire after 1s
+conf.setExpireReadChannelCache(1000);
+conf.setEntryLogFilePreAllocationEnabled(false);
+// below one will cost logId 0
+Bookie bookie = new Bookie(conf);
+// create some entries
+int numLogs = 4;
+int numEntries = 10;
+long[][] positions = new long[numLogs][];
+for (int i = 0; i < numLogs; i++) {
+positions[i] = new long[numEntries];
+EntryLogger logger = new EntryLogger(conf,
+bookie.getLedgerDirsManager());
+for (int j = 0; j < numEntries; j++) {
+positions[i][j] = logger.addEntry(i, generateEntry(i, 
j).nioBuffer());
+}
+logger.flush();
+LOG.info("log id is {}, LeastUnflushedLogId is {} ", 
logger.getCurrentLogId(),
+logger.getLeastUnflushedLogId());
+}
+
+for (int i = 1; i < numLogs + 1; i++) {
+File logFile = new File(curDir, Long.toHexString(i) + ".log");
+assertTrue(logFile.exists());
+}
+
+// create some read for the entry log
+EntryLogger logger = ((InterleavedLedgerStorage) 
bookie.ledgerStorage).entryLogger;
+ThreadLocal>  cacheThreadLocal = 
logger.getLogid2Channel();
+ConcurrentMap 
logid2FileChannel = logger.getLogid2FileChannel();
+for (int j = 0; j < numEntries; j++) {
+logger.readEntry(0, j, positions[0][j]);
+}
+LOG.info("cache size is {}, content is {}", 
cacheThreadLocal.get().size(),
+cacheThreadLocal.get().asMap().toString());
+// the cache has readChannel for 1.log
+assertNotNull(cacheThreadLocal.get().getIfPresent(1L));
+for (int j = 0; j < numEntries; j++) {
+logger.readEntry(1, j, positions[1][j]);
+}
+LOG.info("cache size is {}, content is {}", 
cacheThreadLocal.get().size(),
+cacheThreadLocal.get().asMap().toString());
+// the cache has readChannel for 2.log
+assertNotNull(cacheThreadLocal.get().getIfPresent(2L));
+// expire time
+Thread.sleep(1000);
 
 Review comment:
   ExpireTime semantic is about a period, I think timeout is just like sleep.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r158970434
 
 

 ##
 File path: 
bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/EntryLogTest.java
 ##
 @@ -315,6 +316,104 @@ public void testRecoverFromLedgersMapOnV0EntryLog() 
throws Exception {
 assertEquals(120, meta.getRemainingSize());
 }
 
+/**
+ * Test Cache for logid2Channel and concurrentMap for logid2FileChannel 
work correctly.
+ * Note that, when an entryLogger is initialized, the entry log id will 
increase one.
+ * when the preallocation is enabled, a new entrylogger will cost 2 logId.
+ */
+@Test
+public void testCacheInEntryLog() throws Exception {
+File tmpDir = createTempDir("bkTest", ".dir");
+File curDir = Bookie.getCurrentDirectory(tmpDir);
+Bookie.checkDirectoryStructure(curDir);
+
+int gcWaitTime = 1000;
+ServerConfiguration conf = 
TestBKConfiguration.newServerConfiguration();
+conf.setGcWaitTime(gcWaitTime);
+conf.setLedgerDirNames(new String[] {tmpDir.toString()});
+//since last access, expire after 1s
+conf.setExpireReadChannelCache(1000);
+conf.setEntryLogFilePreAllocationEnabled(false);
+// below one will cost logId 0
+Bookie bookie = new Bookie(conf);
+// create some entries
+int numLogs = 4;
+int numEntries = 10;
+long[][] positions = new long[numLogs][];
+for (int i = 0; i < numLogs; i++) {
+positions[i] = new long[numEntries];
+EntryLogger logger = new EntryLogger(conf,
 
 Review comment:
   that would be good.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r158969960
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1091,24 +1152,30 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 }
 
 private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
+BufferedReadChannel brc = getFromChannels(entryLogId);
+if (brc != null) {
+return brc;
 }
+
 File file = findFile(entryLogId);
 // get channel is used to open an existing entry log file
 // it would be better to open using read mode
 FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
+ReferenceCountedFileChannel oldFc =
+logid2FileChannel.putIfAbsent(entryLogId, new 
ReferenceCountedFileChannel(newFc));
 if (null != oldFc) {
 newFc.close();
-newFc = oldFc;
+newFc = oldFc.fc;
+// increment the refCnt
+oldFc.retain();
 }
+
 // We set the position of the write buffer of this buffered channel to 
Long.MAX_VALUE
 // so that there are no overlaps with the write buffer while reading
-fc = new BufferedReadChannel(newFc, conf.getReadBufferBytes());
-putInReadChannels(entryLogId, fc);
-return fc;
+brc = new BufferedReadChannel(newFc, conf.getReadBufferBytes());
+putInReadChannels(entryLogId, brc);
+LOG.info("put readChannel: {}, corresponding to: {} ", brc, 
entryLogId);
 
 Review comment:
   thanks,fixed


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r157133823
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -1078,25 +1130,26 @@ private Header getHeaderForLogId(long entryLogId) 
throws IOException {
 }
 }
 
+
 private BufferedReadChannel getChannelForLogId(long entryLogId) throws 
IOException {
-BufferedReadChannel fc = getFromChannels(entryLogId);
-if (fc != null) {
-return fc;
+BufferedReadChannel brc = getFromChannels(entryLogId);
+if (brc != null) {
+// increment the refCnt
+logid2FileChannel.get(entryLogId).retain();
+return brc;
 }
+
 File file = findFile(entryLogId);
 // get channel is used to open an existing entry log file
 // it would be better to open using read mode
-FileChannel newFc = new RandomAccessFile(file, "r").getChannel();
-FileChannel oldFc = logid2FileChannel.putIfAbsent(entryLogId, newFc);
-if (null != oldFc) {
-newFc.close();
-newFc = oldFc;
-}
+FileChannel fc = new RandomAccessFile(file, "r").getChannel();
+logid2FileChannel.put(entryLogId, new ReferenceCountedFileChannel(fc));
 
 Review comment:
   fixed


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r157133002
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -327,54 +333,100 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
  * A thread-local variable that wraps a mapping of log ids to 
bufferedchannels
  * These channels should be used only for reading. logChannel is the one
  * that is used for writes.
+ * We use this Guava cache to store the BufferedReadChannel.
+ * When the BufferedReadChannel is removed, the underlying fileChannel's 
refCnt decrease 1,
+ * temporally use 1h to relax replace after reading.
  */
-private final ThreadLocal> logid2Channel =
-new ThreadLocal>() {
+private final ThreadLocal> logid2Channel =
+new ThreadLocal>() {
 @Override
-public Map initialValue() {
+public Cache initialValue() {
 // Since this is thread local there only one modifier
 // We dont really need the concurrency, but we need to use
 // the weak values. Therefore using the concurrency level of 1
-return new MapMaker().concurrencyLevel(1)
-.weakValues()
-.makeMap();
+return CacheBuilder.newBuilder().concurrencyLevel(1)
+.expireAfterAccess(expireReadChannelCacheInHour, 
TimeUnit.HOURS)
+//decrease the refCnt
+.removalListener(removal -> 
logid2FileChannel.get(removal.getKey()).release())
 
 Review comment:
   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.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] ArvinDevel commented on a change in pull request #832: Issue 620: Close the fileChannels for read when they are idle

[GitBox]

ArvinDevel commented on a change in pull request #832: Issue 620: Close the 
fileChannels for read when they are idle
URL: https://github.com/apache/bookkeeper/pull/832#discussion_r156560570
 
 

 ##
 File path: 
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/EntryLogger.java
 ##
 @@ -341,12 +346,43 @@ private int readFromLogChannel(long entryLogId, 
BufferedReadChannel channel, Byt
 }
 };
 
+private final  CacheLoader loader = new 
CacheLoader () {
+public FileChannel load(Long entryLogId) throws Exception {
 
 Review comment:
   @eolivelli  Just as @sijie said, can't use lambda due to it's an abstract 
class


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services