[GitHub] [accumulo] ctubbsii commented on a change in pull request #1459: WIP - Lots of ZooKeeper-related cleanup

[GitBox <gi...@apache.org>]

ctubbsii commented on a change in pull request #1459: WIP - Lots of ZooKeeper-related cleanup
URL: https://github.com/apache/accumulo/pull/1459#discussion_r359157554
 
 

 ##########
 File path: core/src/main/java/org/apache/accumulo/fate/zookeeper/ZooReader.java
 ##########
 @@ -235,9 +135,50 @@ public void sync(final String path) throws KeeperException, InterruptedException
     }, null);
     waiter.await();
     Code code = Code.get(rc.get());
-    if (code != KeeperException.Code.OK) {
+    if (code != Code.OK) {
       throw KeeperException.create(code);
     }
   }
 
+  protected interface ZKFunction<R> extends ZKFunctionMutator<R> {
+    @Override
+    R apply(ZooKeeper zk) throws KeeperException, InterruptedException;
+  }
+
+  protected interface ZKFunctionMutator<R> {
+    R apply(ZooKeeper zk)
+        throws KeeperException, InterruptedException, AcceptableThriftTableOperationException;
+  }
+
+  protected <R> R retryLoop(ZKFunction<R> f) throws KeeperException, InterruptedException {
+    return retryLoopPutData(f, null);
+  }
+
+  protected <R> R retryLoopPutData(ZKFunction<R> f,
+      UnaryOperator<KeeperException> skipRetryIncrement)
+      throws KeeperException, InterruptedException {
+    try {
+      return retryLoopMutator(f, skipRetryIncrement);
+    } catch (AcceptableThriftTableOperationException e) {
+      throw new AssertionError("Don't use this method for whatever you're trying to do.");
+    }
+  }
 
 Review comment:
   No. The method was named after the `ZooReaderWriter.putData()` method, because it is intended to only be used there. The main point is to put the common retry logic in a single place. Unfortunately, each case where we use it, we do weird things. This one needs to suppress the AcceptableThriftTableOperationException that isn't possible to be thrown from the operations in the `putData` method. That exception is only thrown from the `mutate` method, but I didn't want to copy/paste the pattern, with different exception handling, so I just suppress it here. However, it can't use the regular `retryLoop`, because that is a simpler version that doesn't have the special handling for `KeeperException` to switch from trying to create the node to overwriting the node.
   
   It's... complicated... and questionable. I'm taking a few days off of it, and going to revisit with fresh eyes to see if it still makes sense to me, or if there's a better way to write it without a lot of duplicate code.

----------------------------------------------------------------
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


With regards,
Apache Git Services