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 2020/08/27 10:24:53 UTC
[GitHub] [hbase-filesystem] wchevreuil opened a new pull request #14: HBASE-24961 [HBOSS] HBaseObjectStoreSemantics.close should call super.close to make sure its own instance always get removed from FileSystem.CACHE
wchevreuil opened a new pull request #14:
URL: https://github.com/apache/hbase-filesystem/pull/14
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on a change in pull request #14: HBASE-24961 [HBOSS] HBaseObjectStoreSemantics.close should call super.close to make sure its own instance always get removed from FileSystem.CACHE
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #14:
URL: https://github.com/apache/hbase-filesystem/pull/14#discussion_r478729305
##########
File path: hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -180,9 +196,12 @@ protected boolean writeLockAbove(Path p) throws IOException {
LOG.debug("Checking for write lock above {}", p);
while (!p.isRoot()) {
p = p.getParent();
- if (isLocked(get(p).writeLock())) {
- LOG.debug("Parent write lock currently held: {}", p);
- return true;
+ //We need to protect this block against potential concurrent calls to close()
+ synchronized (this) {
Review comment:
I look at this and wonder if it would just be better to mark `writeLockAbove` as `synchronized`, just to avoid all of the lock acquisition and dropping as we go up the directory stack...
```
/hbase/data/default/table/region/
/hbase/data/default/table/
/hbase/data/default/
/hbase/data/
/hbase/
```
I'm not sure if that's going to be a noticeable perf impact. I think if your testing didn't show anything obvious, this is fine for now. Just wanted to write it out.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] joshelser commented on a change in pull request #14: HBASE-24961 [HBOSS] HBaseObjectStoreSemantics.close should call super.close to make sure its own instance always get removed from FileSystem.CACHE
Posted by GitBox <gi...@apache.org>.
joshelser commented on a change in pull request #14:
URL: https://github.com/apache/hbase-filesystem/pull/14#discussion_r480416248
##########
File path: hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -121,9 +120,26 @@ private void waitForCuratorToConnect() {
}
@Override
- public void close() throws IOException {
+ public void close() {
if (curator != null) {
- curator.close();
+ synchronized(this) {
+ curator.close();
+ lockCache.clear();
+ logCaller();
Review comment:
Checked CuratorFrameworkImpl and am happy to see that multiple calls to close() are OK to make.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] wchevreuil merged pull request #14: HBASE-24961 [HBOSS] HBaseObjectStoreSemantics.close should call super.close to make sure its own instance always get removed from FileSystem.CACHE
Posted by GitBox <gi...@apache.org>.
wchevreuil merged pull request #14:
URL: https://github.com/apache/hbase-filesystem/pull/14
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase-filesystem] wchevreuil commented on a change in pull request #14: HBASE-24961 [HBOSS] HBaseObjectStoreSemantics.close should call super.close to make sure its own instance always get removed from FileSystem.CACHE
Posted by GitBox <gi...@apache.org>.
wchevreuil commented on a change in pull request #14:
URL: https://github.com/apache/hbase-filesystem/pull/14#discussion_r478988871
##########
File path: hbase-oss/src/main/java/org/apache/hadoop/hbase/oss/sync/ZKTreeLockManager.java
##########
@@ -180,9 +196,12 @@ protected boolean writeLockAbove(Path p) throws IOException {
LOG.debug("Checking for write lock above {}", p);
while (!p.isRoot()) {
p = p.getParent();
- if (isLocked(get(p).writeLock())) {
- LOG.debug("Parent write lock currently held: {}", p);
- return true;
+ //We need to protect this block against potential concurrent calls to close()
+ synchronized (this) {
Review comment:
Makes sense, let me do it.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org