You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2021/11/10 18:17:01 UTC
[GitHub] [hbase-filesystem] joshelser opened a new pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
joshelser opened a new pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29
HBOSS was orphaning znodes from the src of a path which is renamed. Over
time, this will result in a very large usage of ZK due to HBOSS.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#issuecomment-971115237
Oh, funny. So, my tests do actually clean up ZK, but not for the reason I thought :)
```
2021-11-16 21:20:30,424 [main] DEBUG sync.ZKTreeLockManager (ZKTreeLockManager.java:recursiveDelete(229)) - Removing all mutex and znodes for paths beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,429 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing /s3a/embedded because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing /s3a/embedded/hboss-junit because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing /s3a because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing /s3a/embedded/hboss-junit/dest because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing /s3a/embedded/hboss-junit/src because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:removeInMemoryLocks(253)) - Not removing / because it's not beneath /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,430 [main] DEBUG sync.ZKTreeLockManager (ZKTreeLockManager.java:writeUnlock(160)) - writeLock /s3a/embedded/hboss-junit/src release
2021-11-16 21:20:30,430 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:get(403)) - Lock cached for /s3a/embedded/hboss-junit/src
2021-11-16 21:20:30,434 [main] DEBUG sync.ZKTreeLockManager (ZKTreeLockManager.java:writeUnlock(160)) - writeLock /s3a/embedded/hboss-junit/dest release
2021-11-16 21:20:30,434 [main] INFO sync.ZKTreeLockManager (ZKTreeLockManager.java:get(403)) - Lock cached for /s3a/embedded/hboss-junit/dest
2021-11-16 21:20:30,439 [main] INFO oss.TestZNodeCleanup (TestZNodeCleanup.java:teardown(79)) - Dumping contents of ZooKeeper after test from /s3a/embedded/hboss-junit
2021-11-16 21:20:30,439 [main] INFO oss.TestZNodeCleanup (TestZNodeCleanup.java:printZkBFS(88)) - /s3a/embedded/hboss-junit
2021-11-16 21:20:30,440 [main] INFO oss.TestZNodeCleanup (TestZNodeCleanup.java:printZkBFS(88)) - /s3a/embedded/hboss-junit/.hboss-lock-znode
2021-11-16 21:20:30,440 [main] INFO oss.TestZNodeCleanup (TestZNodeCleanup.java:printZkBFS(88)) - /s3a/embedded/hboss-junit/dest
2021-11-16 21:20:30,441 [main] INFO oss.TestZNodeCleanup (TestZNodeCleanup.java:printZkBFS(88)) - /s3a/embedded/hboss-junit/dest/.hboss-lock-znode
```
This is the logging I added after a successful `mv /src /dest`. We clean up the corresponding node for `/src` in ZK for HBOSS as expected, but I still got `removeInMemoryLocks()` wrong in that I don't remove the Mutex for the parent `/src` znode. That's a bug and the same one I thought I fixed. Go figure.
However, because we don't remove that parent src lock, we can call release on it _without_ recreating the state in ZK. However, we do orphan the Mutex inside Curator/HBOSS which is no good. However, I think this also happens for lockDelete, so I think it might be best to fix that separately. Let me sleep on it and see what I think in the morning.
Thanks for your careful eyes, Wellington. Curious what you think, too.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#issuecomment-967567313
`mvn package -Pzk && mvn package -Pzk -Dhadoop.profile=3.2` passed. It looks like `-Plocal` and `-Pnull` may have other issues which need to be corrected.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] wchevreuil commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r747565792
##########
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:
I think it's ok to delete it. The only con is that anyone trying to access it again would then recreate the znode in writeLock()/readLock().
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r746864829
##########
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:
One thing I'm worried about after looking at this (for the third time): is it safe to delete the znodes on the src if the rename was unsuccessful? I need to think about this.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#issuecomment-967583858
> It looks like -Plocal and -Pnull may have other issues which need to be corrected.
Oh, I'm silly :). `-Pnull` would be expected to fail. `-Plocal` seems to have passed on re-run so maybe a race condition to address later.
I think this is ready to go.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser closed pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser closed pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r750826981
##########
File path: hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -161,7 +161,7 @@ protected void writeUnlock(Path p) throws IOException {
get(p).writeLock().release();
Review comment:
Yeah, right as usual. I want to figure out why my unit test isn't failing like I think it should, given this.
However, I think I can interpret you see this as an improvement, but there is more work to be done (citing the problems, but approving this PR)? Just wanted to double check.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
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: issues-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] wchevreuil commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#discussion_r750478969
##########
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:
nit: typo "iff"
##########
File path: hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -161,7 +161,7 @@ protected void writeUnlock(Path p) throws IOException {
get(p).writeLock().release();
Review comment:
This 'get' call creates the znode if it's not in the cache, so call to writeUnlock after deleting the znode would create it again.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
joshelser commented on pull request #29:
URL: https://github.com/apache/hbase-filesystem/pull/29#issuecomment-967479992
I put both HBASE-26437 and HBASE-26453 into this one PR (as I can't test the latter without the former).
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] wchevreuil commented on a change in pull request #29: HBASE-26437 Clean up the znodes for the src after a rename.
Posted by GitBox <gi...@apache.org>.
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org