[GitHub] [accumulo] ctubbsii commented on a change in pull request #2546: Created a synchronous ZooKeeper.sync() method for use in ZooKeeperTestingServer tests

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

ctubbsii commented on a change in pull request #2546:
URL: https://github.com/apache/accumulo/pull/2546#discussion_r821681635



##########
File path: test/src/main/java/org/apache/accumulo/test/zookeeper/ZooKeeperTestingServer.java
##########
@@ -41,6 +46,31 @@
  */
 public class ZooKeeperTestingServer implements AutoCloseable {
 
+  public static class ZooKeeperForTests extends ZooKeeper {
+
+    public ZooKeeperForTests(String connectString, int sessionTimeout, Watcher watcher)
+        throws IOException {
+      super(connectString, sessionTimeout, watcher);
+    }
+
+    @Override
+    public void sync(final String path, VoidCallback cb, Object ctx) {
+      PathUtils.validatePath(path);
+

Review comment:
       I don't think it's a good idea to create a subclass that effectively changes the behavior of `sync` to diverge from the behavior documented in the API javadoc for the base class. I also don't think it's a good idea to call the ZK internals in here, which are not necessarily stable across versions. I have another idea, I'm going to put up an alternative PR that uses a custom callback that I think is simpler.




-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org