yzang commented on a change in pull request #913: Refactor FileInfo locking and
refcounting out IndexPersistenceMgr
URL: https://github.com/apache/bookkeeper/pull/913#discussion_r158855658
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/IndexPersistenceMgr.java
##########
@@ -174,90 +170,57 @@ public Number getSample() {
return builder.build();
}
+ private File createFileInfoBackingFile(long ledger, boolean
createIfMissing) throws IOException {
+ File lf = findIndexFile(ledger);
+ if (null == lf) {
+ if (!createIfMissing) {
+ throw new Bookie.NoLedgerException(ledger);
+ }
+ // We don't have a ledger index file on disk or in cache, so
create it.
+ lf = getNewLedgerIndexFile(ledger, null);
+ }
+ return lf;
+ }
+
/**
* When a ledger is evicted, we need to make sure there's no other thread
* trying to get FileInfo for that ledger at the same time when we close
* the FileInfo.
*/
- private void handleLedgerEviction(RemovalNotification<Long, FileInfo>
notification) {
- FileInfo fileInfo = notification.getValue();
+ private void handleLedgerEviction(RemovalNotification<Long,
CachedFileInfo> notification) {
+ CachedFileInfo fileInfo = notification.getValue();
Long ledgerId = notification.getKey();
if (null == fileInfo || null == notification.getKey()) {
return;
}
if (notification.wasEvicted()) {
evictedLedgersCounter.inc();
- // we need to acquire the write lock in another thread,
- // otherwise there could be dead lock happening.
- evictionThreadPool.execute(() -> {
- fileInfoLock.writeLock().lock();
- try {
- // We only close the fileInfo when we evict the FileInfo
from both cache
- if (!readFileInfoCache.asMap().containsKey(ledgerId)
- &&
!writeFileInfoCache.asMap().containsKey(ledgerId)) {
- fileInfo.close(true);
- }
- } catch (IOException e) {
- LOG.error("Exception closing file info when ledger {} is
evicted from file info cache.",
- ledgerId, e);
- } finally {
- fileInfoLock.writeLock().unlock();
- }
- });
}
fileInfo.release();
}
- class FileInfoLoader implements Callable<FileInfo> {
-
- final long ledger;
- final byte[] masterKey;
-
- FileInfoLoader(long ledger, byte[] masterKey) {
- this.ledger = ledger;
- this.masterKey = masterKey;
- }
-
- @Override
- public FileInfo call() throws IOException {
- File lf = findIndexFile(ledger);
- if (null == lf) {
- if (null == masterKey) {
- throw new Bookie.NoLedgerException(ledger);
- }
- // We don't have a ledger index file on disk or in cache, so
create it.
- lf = getNewLedgerIndexFile(ledger, null);
- activeLedgers.put(ledger, true);
- }
- FileInfo fi = new FileInfo(lf, masterKey);
- fi.use();
- return fi;
- }
- }
-
/**
* Get the FileInfo and increase reference count.
* When we get FileInfo from cache, we need to make sure it is synchronized
* with eviction, otherwise there might be a race condition as we get
* the FileInfo from cache, that FileInfo is then evicted and closed
before we
* could even increase the reference counter.
*/
- FileInfo getFileInfo(final Long ledger, final byte masterKey[]) throws
IOException {
+ CachedFileInfo getFileInfo(final Long ledger, final byte masterKey[])
throws IOException {
try {
- FileInfo fi;
+ CachedFileInfo fi;
pendingGetFileInfoCounter.inc();
- fileInfoLock.readLock().lock();
+ Callable<CachedFileInfo> loader = () -> {
+ CachedFileInfo fileInfo =
fileInfoBackingCache.loadFileInfo(ledger, masterKey);
+ activeLedgers.put(ledger, true);
+ return fileInfo;
+ };
if (null != masterKey) {
- fi = writeFileInfoCache.get(ledger,
- new FileInfoLoader(ledger, masterKey));
- if (null == readFileInfoCache.asMap().putIfAbsent(ledger, fi))
{
- fi.use();
- }
+ fi = writeFileInfoCache.get(ledger, loader);
Review comment:
Why would it increment refCount by 2? The function who call getFileInfo will
release the fileInfo immediately in the finally block.
----------------------------------------------------------------
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:
[email protected]
With regards,
Apache Git Services
