joshelser commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r746943784
##########
File path:
hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/TreeLockManager.java
##########
@@ -488,7 +488,18 @@ public void close() throws IOException {
try {
writeUnlock(src);
} finally {
- writeUnlock(dst);
+ try {
+ writeUnlock(dst);
+ } finally {
+ // 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.
+ recursiveDelete(src);
Review comment:
Yeah, this is wrong, as-is. I can't be altering ZK state after giving up
the write lock. Needs to be more like `lockDelete`.
##########
File path:
hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -252,12 +253,31 @@ private synchronized void removeInMemoryLocks(Path p) {
}
}
- private boolean isBeneath(Path parent, Path other) {
- if (parent.equals(other)) {
+ /**
+ * Returns true iff the given path is contained beneath the parent path.
Review comment:
Oh, this is intentional. Shorthand for "if and only if"
##########
File path:
hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/TreeLockManager.java
##########
@@ -488,7 +488,18 @@ public void close() throws IOException {
try {
writeUnlock(src);
} finally {
- writeUnlock(dst);
+ try {
+ writeUnlock(dst);
+ } finally {
+ // 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.
+ recursiveDelete(src);
Review comment:
Ok, got this working by deleting the children of the parent prior to
releasing the write lock. The naive fix wasn't working because of HBASE-26453,
not because I was deleting the children prior :)
##########
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 calls get, which in
turn creates the znode.
Crap.
> I guess lockDelete also has same problem.
Yeah, I had copied what `lockDelete` does in the hopes that it was doing the
right thing :)
> I'm not sure now how curator manages the locks. Is it bound to the
specific znode it was created for
So, I think the reason that this works is that Curator is creating znodes
beneath the .hboss-lock-node. There's a specially formatted znode with READ or
WRITE in the name when there is a held lock. I'll have to look into how the
heck the unit test is passing...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
