[GitHub] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176671260
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
##
@@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo
fileInfo) {
}
void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
Review comment:
done. changed to use `UncheckedIOException`
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] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176669917
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
##
@@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo
fileInfo) {
}
void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
Review comment:
okay will change that.
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] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176669548
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
##
@@ -52,25 +63,29 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[]
masterKey) throws IOException
// have been able to get a reference to it here.
// The caller of loadFileInfo owns the refence, and is
// responsible for calling the corresponding #release().
-boolean retained = fi.tryRetain();
-assert(retained);
-return fi;
+return tryRetainFileInfo(fi);
}
} finally {
lock.readLock().unlock();
}
+File backingFile = fileLoader.load(ledgerId, masterKey != null);
+CachedFileInfo newFi = new CachedFileInfo(ledgerId, backingFile,
masterKey);
Review comment:
> The race exists according to the contract provided by
FileInfoBackingCache. I'm pretty sure it can happen with the cache loaders
also, though that should be irrelevant to the discussion, as whether a
container is safe should make it irrelevant as to how it is used.
this is not a general class. this is a class for the file info cache. you
are taking how fileinfo and fileinfo cache work into account. your example is
the exact example showing this class is related to fileinfo and fileinfo cache.
in your new example, you are trying to say "fileInfo does not appear to be
fenced", how's that possible. all the fencing bit are lazily loaded via
FileInfo#checkOpen. At the point Read Fi CacheLoader put the fileinfo into the
cache, both write and read fi cache loader use same file info object, which
will get exact fence bit.
> But one thread can modify the state and persist it back, and the other
thread would be unaware of these state changes.
how's possible? I only move the construction and file lookup out of the
write lock, this doesn't change the original behavior. the get and put are
under the write lock, read & write fi cache will reference same FiInfo when
this file info exist in both case.
the only consider would only coming from file lookup is out of the lock, and
I have explained this wasn't a problem in my previous comment.
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] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176535276
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
##
@@ -52,25 +63,29 @@ CachedFileInfo loadFileInfo(long ledgerId, byte[]
masterKey) throws IOException
// have been able to get a reference to it here.
// The caller of loadFileInfo owns the refence, and is
// responsible for calling the corresponding #release().
-boolean retained = fi.tryRetain();
-assert(retained);
-return fi;
+return tryRetainFileInfo(fi);
}
} finally {
lock.readLock().unlock();
}
+File backingFile = fileLoader.load(ledgerId, masterKey != null);
+CachedFileInfo newFi = new CachedFileInfo(ledgerId, backingFile,
masterKey);
Review comment:
I am not sure the race condition you describe exists.
1), this method is only called by guava cache when loading the file info.
guava cache guarantees [awaiting pending load for a given
key](https://google.github.io/guava/releases/23.0/api/docs/com/google/common/cache/Cache.html#get-K-java.util.concurrent.Callable-).
so thread a and thread b can not happen on one guava cache. **so one has to be
loading from write fi cache, the other one has to be loading from read fi
cache. **
2), the file info loader is basically doing file lookup and construct the
file info objects. it doesn't touch any persistence state (e.g. create file).
so if the file exists at disk, the file info object created by thread a and
thread b are identical. so there is no race condition. so the only concern is
file doesn't exist on disk, ThreadA picks a directory while ThreadB picks
another directory.
now lets talk about "ThreadA picks one directory while ThreadB picks another
directory".
in this case, both ThreadA and ThreadB can't find file on the disk, it means
it is a first time creation. a read request can only attempt to load file info
after a write request creates a file info.
https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/HandleFactoryImpl.java#L61
so this case won't actually happen.
hope this clarify.
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] sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
sijie commented on a change in pull request #1284: Improve FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176520016
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/FileInfoBackingCache.java
##
@@ -92,8 +107,20 @@ private void releaseFileInfo(long ledgerId, CachedFileInfo
fileInfo) {
}
void closeAllWithoutFlushing() throws IOException {
-for (Map.Entry entry : fileInfos.entrySet()) {
-entry.getValue().close(false);
+try {
Review comment:
I don't think we should change the signature to throw unchecked io
exception. checked exception is well handled in the whole path, but not
unchecked exception.
The logic here is just to getting around this interface doesn't throw
checked exception, so I am able to throw checked exception. there is no really
matter it is unchecked execution exception or unchecked io exception.
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
