You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2021/02/01 19:12:50 UTC
[GitHub] [accumulo] dlmarion opened a new pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
dlmarion opened a new pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568108805
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ private void recursiveDelete(String zPath, NodeMissingPolicy policy)
+ throws KeeperException, InterruptedException {
+ if (policy == NodeMissingPolicy.CREATE) {
+ throw new IllegalArgumentException(policy.name() + " is invalid for this operation");
+ }
+ try {
+ // delete children
+ for (String child : zooKeeper.getChildren(zPath, null)) {
+ recursiveDelete(zPath + "/" + child, NodeMissingPolicy.SKIP);
+ }
+
+ // delete self
+ zooKeeper.delete(zPath, -1);
+ } catch (KeeperException e) {
+ // new child appeared; try again
+ if (e.code() == Code.NOTEMPTY) {
+ recursiveDelete(zPath, policy);
+ }
+ if (policy == NodeMissingPolicy.SKIP && e.code() == Code.NONODE) {
+ return;
+ }
+ throw e;
+ }
+ }
+
Review comment:
NodeMissingPolicy is for us to throw an exception if we try to delete something that we expect to exist but doesn't. I'm not sure if it would be useful here or not.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568215173
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(zooKeeper, pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ public static void recursiveDelete(ZooKeeper zooKeeper, String zPath, NodeMissingPolicy policy)
Review comment:
ZooUtil probably makes more sense for this utility, since it's not ZooLock-specific code.
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -90,21 +92,18 @@ public String toString() {
private String createdNodeName;
private String watchingNodeName;
- public ZooLock(ZooReaderWriter zoo, String path, UUID uuid) {
- this(new ZooCache(zoo), zoo, path, uuid);
+ public ZooLock(AccumuloConfiguration conf, String path, UUID uuid) {
+ this(conf.get(Property.INSTANCE_ZK_HOST),
+ (int) conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
+ conf.get(Property.INSTANCE_SECRET), path, uuid);
}
public ZooLock(String zookeepers, int timeInMillis, String secret, String path, UUID uuid) {
Review comment:
The only place this constructor is called is in ZooLockIT, with the same args over and over again. It would be simpler to delete this constructor, and update ZooLockIT use a ConfigurationCopy instance of AccumuloConfiguration with the config properties already set up in a `@Before` method, so ZooLockIT can just use the other constructor.
----------------------------------------------------------------
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] [accumulo] ctubbsii commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568215173
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(zooKeeper, pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ public static void recursiveDelete(ZooKeeper zooKeeper, String zPath, NodeMissingPolicy policy)
Review comment:
ZooUtil probably makes more sense for this utility, since it's not ZooLock-specific code.
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -90,21 +92,18 @@ public String toString() {
private String createdNodeName;
private String watchingNodeName;
- public ZooLock(ZooReaderWriter zoo, String path, UUID uuid) {
- this(new ZooCache(zoo), zoo, path, uuid);
+ public ZooLock(AccumuloConfiguration conf, String path, UUID uuid) {
+ this(conf.get(Property.INSTANCE_ZK_HOST),
+ (int) conf.getTimeInMillis(Property.INSTANCE_ZK_TIMEOUT),
+ conf.get(Property.INSTANCE_SECRET), path, uuid);
}
public ZooLock(String zookeepers, int timeInMillis, String secret, String path, UUID uuid) {
Review comment:
The only place this constructor is called is in ZooLockIT, with the same args over and over again. It would be simpler to delete this constructor, and update ZooLockIT use a ConfigurationCopy instance of AccumuloConfiguration with the config properties already set up in a `@Before` method, so ZooLockIT can just use the other constructor.
----------------------------------------------------------------
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] [accumulo] dlmarion merged pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
dlmarion merged pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896
----------------------------------------------------------------
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] [accumulo] EdColeman commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568099196
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ private void recursiveDelete(String zPath, NodeMissingPolicy policy)
+ throws KeeperException, InterruptedException {
+ if (policy == NodeMissingPolicy.CREATE) {
+ throw new IllegalArgumentException(policy.name() + " is invalid for this operation");
+ }
+ try {
+ // delete children
+ for (String child : zooKeeper.getChildren(zPath, null)) {
+ recursiveDelete(zPath + "/" + child, NodeMissingPolicy.SKIP);
+ }
+
+ // delete self
+ zooKeeper.delete(zPath, -1);
+ } catch (KeeperException e) {
+ // new child appeared; try again
+ if (e.code() == Code.NOTEMPTY) {
+ recursiveDelete(zPath, policy);
+ }
+ if (policy == NodeMissingPolicy.SKIP && e.code() == Code.NONODE) {
+ return;
+ }
+ throw e;
+ }
+ }
+
Review comment:
With ZooKeeper 3.5.x there is a static utility method `ZKUtil.deleteRecursive(zooKeeper, path);` that may eliminate the need for most, if not all of this code. (I'm not sure what the NodeMissingPolicy is expected to do here, so that may be a factor)
----------------------------------------------------------------
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] [accumulo] EdColeman commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568103198
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -519,7 +552,7 @@ public synchronized boolean tryToCancelAsyncLockOrUnlock()
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Deleting all at path {} due to lock cancellation", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
Review comment:
See other comment regarding `ZKUtil.deleteRecursive(zooKeeper, pathToDelete)`
----------------------------------------------------------------
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] [accumulo] EdColeman commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
EdColeman commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568099196
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ private void recursiveDelete(String zPath, NodeMissingPolicy policy)
+ throws KeeperException, InterruptedException {
+ if (policy == NodeMissingPolicy.CREATE) {
+ throw new IllegalArgumentException(policy.name() + " is invalid for this operation");
+ }
+ try {
+ // delete children
+ for (String child : zooKeeper.getChildren(zPath, null)) {
+ recursiveDelete(zPath + "/" + child, NodeMissingPolicy.SKIP);
+ }
+
+ // delete self
+ zooKeeper.delete(zPath, -1);
+ } catch (KeeperException e) {
+ // new child appeared; try again
+ if (e.code() == Code.NOTEMPTY) {
+ recursiveDelete(zPath, policy);
+ }
+ if (policy == NodeMissingPolicy.SKIP && e.code() == Code.NONODE) {
+ return;
+ }
+ throw e;
+ }
+ }
+
Review comment:
With ZooKeeper 3.5.x there is a static utility method `ZKUtil.deleteRecursive(zooKeeper, path);` that may eliminate the need for most, if not all of this code. (I'm not sure what the NodeMissingPolicy is expected to do here, so that may be a factor)
Also, not sure if it would be possible / desirable, but maybe some kind of sanity check before calling a recursive delete? One thing that comes to mind is checking that the path starts with /accumulo/[uuid]/lock or whatever the appropriate base path for the lock is?
----------------------------------------------------------------
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] [accumulo] dlmarion commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568122017
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ private void recursiveDelete(String zPath, NodeMissingPolicy policy)
+ throws KeeperException, InterruptedException {
+ if (policy == NodeMissingPolicy.CREATE) {
+ throw new IllegalArgumentException(policy.name() + " is invalid for this operation");
+ }
+ try {
+ // delete children
+ for (String child : zooKeeper.getChildren(zPath, null)) {
+ recursiveDelete(zPath + "/" + child, NodeMissingPolicy.SKIP);
+ }
+
+ // delete self
+ zooKeeper.delete(zPath, -1);
+ } catch (KeeperException e) {
+ // new child appeared; try again
+ if (e.code() == Code.NOTEMPTY) {
+ recursiveDelete(zPath, policy);
+ }
+ if (policy == NodeMissingPolicy.SKIP && e.code() == Code.NONODE) {
+ return;
+ }
+ throw e;
+ }
+ }
+
Review comment:
@edcoleman @ctubbsii - the `recursiveDelete` method here is just a copy of the ZooReaderWriter.recursiveDelete method. Looking at the ZKUtil.recursiveDelete it appears that this method with the NodeMissingPolicy.SKIP option might be better. The ZKUtil.recursiveDelete might throw an error in a concurrent delete case.
----------------------------------------------------------------
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] [accumulo] dlmarion commented on a change in pull request #1896: re #1086: modify ZooLock to use the native zookeeper client
Posted by GitBox <gi...@apache.org>.
dlmarion commented on a change in pull request #1896:
URL: https://github.com/apache/accumulo/pull/1896#discussion_r568131633
##########
File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooLock.java
##########
@@ -158,13 +157,41 @@ public synchronized boolean tryLock(LockWatcher lw, byte[] data)
String pathToDelete = path + "/" + createdNodeName;
LOG.debug("[{}] Failed to acquire lock in tryLock(), deleting all at path: {}", vmLockPrefix,
pathToDelete);
- zooKeeper.recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
+ recursiveDelete(pathToDelete, NodeMissingPolicy.SKIP);
createdNodeName = null;
}
return false;
}
+ /**
+ * This method will delete a node and all its children.
+ */
+ private void recursiveDelete(String zPath, NodeMissingPolicy policy)
+ throws KeeperException, InterruptedException {
+ if (policy == NodeMissingPolicy.CREATE) {
+ throw new IllegalArgumentException(policy.name() + " is invalid for this operation");
+ }
+ try {
+ // delete children
+ for (String child : zooKeeper.getChildren(zPath, null)) {
+ recursiveDelete(zPath + "/" + child, NodeMissingPolicy.SKIP);
+ }
+
+ // delete self
+ zooKeeper.delete(zPath, -1);
+ } catch (KeeperException e) {
+ // new child appeared; try again
+ if (e.code() == Code.NOTEMPTY) {
+ recursiveDelete(zPath, policy);
+ }
+ if (policy == NodeMissingPolicy.SKIP && e.code() == Code.NONODE) {
+ return;
+ }
+ throw e;
+ }
+ }
+
Review comment:
I'm going to add another commit here shortly to remove the dupe method.
----------------------------------------------------------------
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