ctubbsii commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1337759945
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:
##########
@@ -2708,6 +2708,31 @@ public void
getEphemerals(AsyncCallback.EphemeralsCallback cb, Object ctx) {
getEphemerals("/", cb, ctx);
}
+ /**
+ * Synchronous sync. Flushes channel between process and leader.
+ *
+ * @param path the given path
+ * @throws KeeperException If the server signals an error with a non-zero
error code
+ * @throws InterruptedException If the server transaction is interrupted.
+ * @throws IllegalArgumentException if an invalid path is specified
+ */
+ public void sync(final String path) throws KeeperException,
InterruptedException {
+ final String clientPath = path;
+ PathUtils.validatePath(clientPath);
+
+ final String serverPath = prependChroot(clientPath);
+
+ RequestHeader h = new RequestHeader();
+ h.setType(ZooDefs.OpCode.sync);
+ SyncRequest request = new SyncRequest();
+ SyncResponse response = new SyncResponse();
+ request.setPath(serverPath);
Review Comment:
It would be cleaner to refactor this to share code with the asynchronous
version, since everything up to here is identical.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:
##########
@@ -2708,6 +2708,31 @@ public void
getEphemerals(AsyncCallback.EphemeralsCallback cb, Object ctx) {
getEphemerals("/", cb, ctx);
}
+ /**
+ * Synchronous sync. Flushes channel between process and leader.
+ *
+ * @param path the given path
+ * @throws KeeperException If the server signals an error with a non-zero
error code
+ * @throws InterruptedException If the server transaction is interrupted.
+ * @throws IllegalArgumentException if an invalid path is specified
+ */
+ public void sync(final String path) throws KeeperException,
InterruptedException {
Review Comment:
Is the path argument even needed? My understanding is that the path has no
effect.
##########
zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java:
##########
@@ -2708,6 +2708,31 @@ public void
getEphemerals(AsyncCallback.EphemeralsCallback cb, Object ctx) {
getEphemerals("/", cb, ctx);
}
+ /**
+ * Synchronous sync. Flushes channel between process and leader.
+ *
+ * @param path the given path
+ * @throws KeeperException If the server signals an error with a non-zero
error code
+ * @throws InterruptedException If the server transaction is interrupted.
+ * @throws IllegalArgumentException if an invalid path is specified
+ */
+ public void sync(final String path) throws KeeperException,
InterruptedException {
+ final String clientPath = path;
+ PathUtils.validatePath(clientPath);
+
+ final String serverPath = prependChroot(clientPath);
+
+ RequestHeader h = new RequestHeader();
+ h.setType(ZooDefs.OpCode.sync);
+ SyncRequest request = new SyncRequest();
+ SyncResponse response = new SyncResponse();
+ request.setPath(serverPath);
+ ReplyHeader r = cnxn.submitRequest(h, request, response, null);
+ if (r.getErr() != 0) {
+ throw KeeperException.create(KeeperException.Code.get(r.getErr()),
clientPath);
Review Comment:
When an asynchronous method and a synchronous version are available, I feel
like the easiest implementation of a synchronous version is something like:
1. Call the asynchronous version, and return a Future
2. Make the current thread wait on the completion of the Future that was
returned in step 1
This implementation might be doing something like that, but it's not obvious
as written. If it's doing anything like that, the details might be obscured
inside the submitRequest method. That might make this implementation less
readable, and therefore harder to maintain. I think using Futures are more
intuitive, if it's not too difficult to implement that way.
You actually have a version like this in the test code in this PR. I'm not
sure why that couldn't be the implementation here.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
