wchevreuil commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r750139620
##########
File path:
hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -241,7 +242,7 @@ protected void recursiveDelete(Path p) throws IOException {
}
}
- private synchronized void removeInMemoryLocks(Path p) {
+ synchronized void removeInMemoryLocks(Path p) {
Iterator<Entry<Path,InterProcessReadWriteLock>> iter =
lockCache.entrySet().iterator();
while (iter.hasNext()) {
Entry<Path,InterProcessReadWriteLock> entry = iter.next();
Review comment:
We may need to explicitly check if we have lock and release it here. See
my comment above.
##########
File path:
hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/TreeLockManager.java
##########
@@ -484,8 +487,29 @@ public AutoLock lockRename(Path rawSrc, Path rawDst)
throws IOException {
}
return new AutoLock() {
public void close() throws IOException {
+ // We have to clean up the src znodes:
+ // 1. If the rename was successful
+ // 2. While we still hold the write lock
LOG.debug("About to unlock after rename: from {} to {}", src, dst);
try {
+ Boolean renameSuccess;
+ try {
+ renameSuccess = successFuture.get();
+ } catch (InterruptedException | ExecutionException e) {
+ LOG.warn("Unable to determine if filesystem rename was successful.
Assuming it failed.", e);
+ renameSuccess = false;
+ }
+ if (renameSuccess != null && renameSuccess.booleanValue()) {
+ // Tricky... HBossContract tests tough things like
+ // `rename("/", "/somethingelse")`
+ // This means we grabbed write locks on
+ // / (src)
+ // /somethingelse (dst)
+ // Thus, we can't safely delete the znodes for src as it may
+ // then also affect the (held) lock on the dst. This is why
+ // we only delete the znodes on success.
+ recursiveDelete(src);
+ }
writeUnlock(src);
Review comment:
This call would recreate the znode, as
[writeUnlock](https://github.com/apache/hbase-filesystem/blob/f21af3719555a6a0c7c060195f92639690b94423/hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java#L161)
calls
[get](https://github.com/apache/hbase-filesystem/blob/f21af3719555a6a0c7c060195f92639690b94423/hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java#L368),
which in turn creates the znode.
I guess lockDelete also has same problem.
I'm not sure now how curator manages the locks. Is it bound to the specific
znode it was created for, so that if the znode got deleted, the lock is gone?
If so, we don't need to worry about unlock it after recursiveDelete. Otherwise,
we would need to change `ZKTreeLockManager.removeInMemoryLocks` to explicitly
release the locks when removing it from the memory cache.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
