[GitHub] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176673561
##
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:
ah, I had it in my head that the loader actually read the file, but it
should have been obvious it didn't.
This change works fine then. Why not get rid of the lock completely though?
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] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176658955
##
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:
> there is no really matter it is unchecked execution exception or unchecked
io exception.
The difference is the instanceof. With UncheckedIOException:
```
try {
// blah
} catch (UncheckedIOException uio) {
throw uio.getCause(); // returns a IOException
}
```
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] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176658666
##
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.
```
| Read Fi CacheLoader| Write Fi CacheLoader
|
| == |
==|
| calls loadInfoInfo |
|
| - checks fileInfos under read lock |
|
| - loads fileinfo for file |
|
|| calls loadFileInfo
|
|| - checks fileInfos under read
lock|
|| - loads fileinfo for file
|
|| - puts into fileInfos under
write lock|
||
|
|| // does something else, maybe
fencing |
||
|
|| calls releaseFileInfo
|
|| - flushes out fencing flag
|
|| - removes from fileInfo
|
| - puts info fileInfos under write lock |
|
|| calls loadFileInfo
|
|| - checks fileInfos under read
lock|
|| - finds fileInfo loaded from
other thread |
|| - fileInfo does not appear to
be fenced |
||
|
```
> the file info object created by thread a and thread b are identical.
At the time of creation yes. But one thread can modify the state and persist
it back, and the other thread would be unaware of these state changes.
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] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176516627
##
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:
ya, saw that later. Better to use UncheckedIOException in any case. Maybe
even change FileInfo close to throw 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] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176476938
##
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 put the load inside the lock for a reason. This change introduces a race.
```
| ThreadA| ThreadB
|
| == |
== |
| calls loadInfoInfo |
|
| - checks fileInfos under read lock |
|
| - loads fileinfo for file |
|
|| calls loadFileInfo
|
|| - checks fileInfos under read
lock |
|| - loads fileinfo for file
|
|| - puts into fileInfos under
write lock |
||
|
|| // does something else, maybe
fencing |
||
|
|| calls releaseFileInfo
|
|| - flushes out fencing flag
|
|| - removes from fileInfo
|
| - puts info fileInfos under write lock |
|
```
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] ivankelly commented on a change in pull request #1284: Improve FileInfoBackingCache
ivankelly commented on a change in pull request #1284: Improve
FileInfoBackingCache
URL: https://github.com/apache/bookkeeper/pull/1284#discussion_r176473048
##
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 see the point of this change. I'm pretty sure used a foreach loop in
first place to avoid something like 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
