ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains #2068

kezhuw · 2023-09-26T16:52:12Z

Previously, there is no synchronous sync so client has to convert asynchronous sync a bit to fit synchronous call chains. This is apparently unfriendly.

Besides above, in absent of ZOOKEEPER-22, we can't issue a fire and forget asynchronous sync to gain strong consistent. So it becomes crucial for client to have a convenient synchronous sync.

Refs: ZOOKEEPER-1167, ZOOKEEPER-4747

Previously, there is no synchronous `sync` so client has to convert asynchronous `sync` a bit to fit synchronous call chains. This is apparently unfriendly. Besides above, in absent of ZOOKEEPER-22, we can't issue a fire and forget asynchronous `sync` to gain strong consistent. So it becomes crucial for client to have a convenient synchronous `sync`. Refs: ZOOKEEPER-1167, ZOOKEEPER-4747

ctubbsii · 2023-09-26T20:42:16Z

zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java

+     * @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 {


Is the path argument even needed? My understanding is that the path has no effect.

Yeh, it does no take effect since day one of sync. ZOOKEEPER-3414(#1187) proposed to error on no node. I think we should keep it until we have conclusion about that and semantics of sync path. Otherwise, if ZOOKEEPER-3414 is delivered in future, the sync with no path(default to "/") could fail due to chroot is not required to be existed in data tree. At the least, I think it is a separate issue and we should keep consistent between two versions of sync.

I understand the argument for keeping it consistent, but I also know it's a pain to have API churn. So, it'd be nice for a decision to be reached and to add the new method in just once, rather than add it, and have to change it (or add a confusing overloaded version) later.

ctubbsii · 2023-09-26T20:43:24Z

zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java

+        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);


It would be cleaner to refactor this to share code with the asynchronous version, since everything up to here is identical.

I guess it may not worth the effort.

To cover only above code, we probably have to introduce wrapper struct to cover context for submitRequest and queuePacket.

To cover also following code, we probably have to introduce an overload to accept all parameters from asynchronous code and a flag to differentiate origin as zk.sync("/", null, null") is valid.

A little duplication probably be a good here.

ctubbsii · 2023-09-26T20:47:38Z

zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java

+        ReplyHeader r = cnxn.submitRequest(h, request, response, null);
+        if (r.getErr() != 0) {
+            throw KeeperException.create(KeeperException.Code.get(r.getErr()), clientPath);


When an asynchronous method and a synchronous version are available, I feel like the easiest implementation of a synchronous version is something like:

Call the asynchronous version, and return a Future

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.

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.

I am positive to using future internally for synchronous api. This should also solve your above concern about "identical code". But I found ZOOKEEPER-4749 in investigation, may be we should wait a minute. I don't want to handle it specially for sole this api.

eolivelli

LGTM

while I understand @ctubbsii concerns, actually the patch this way looks simpler

…us api Currently, `zookeeper.request.timeout` is only respected in synchronous api. I think there are should be no much differences between following two. ```java String createdPath = zk.create("/path", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT); ``` ```java CompletableFuture<String> future = new CompletableFuture<>(); zk.create("/path", data, Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, (rc, path, ctx, name) -> { if (rc == 0) { future.complete(name); } else { future.completeExceptionally(KeeperException.create(KeeperException.Code.get(rc), path)); } }, null); String createdPath = future.join(); ``` After this pr, we are able to unify synchronous api through calls to asynchronous api as [review comments][review-comments] pointed out if we feel there are too much identical code between synchronous and asynchronous api. [review-comments]: apache#2068 (comment)

kezhuw · 2024-08-26T06:04:14Z

I plan to merge this now as all remaining review comments are either covered or cloud covered by separated jiras now:

ZOOKEEPER-3414 proposes way to handle path in sync.
ZOOKEEPER-4749(ZOOKEEPER-4749: Respect zookeeper.request.timeout also for asynchronous api #2168) tries to unify zookeeper.request.timeout semantics between synchronous and asynchronous code. I believe we could easily refactor synchronous code using asynchronous apis once this is solved.

anmolnar · 2024-09-18T20:42:16Z

@kezhuw @eolivelli Are u sure you don't want this in 3.9 release line?
I see it's new API, but since it's not breaking change, we might want to backport it.

kezhuw · 2024-09-20T12:30:44Z

Do we ever guarantee forward compatibility for released patch versions ?

If not, I am positive for this to be backported. The code is simple and the functionality is almost a simple wrapper for asynchronous version.

We just have encountered that netty breaks forward compatibility (#1917 (comment)). :-)

anmolnar · 2024-09-20T18:53:24Z

Yes, we only have backward compatiblity rules, but not forward compatibility.

…2068) Previously, there is no synchronous `sync` so client has to convert asynchronous `sync` a bit to fit synchronous call chains. This is apparently unfriendly. Besides above, in absent of ZOOKEEPER-22, we can't issue a fire and forget asynchronous `sync` to gain strong consistent. So it becomes crucial for client to have a convenient synchronous `sync`. Refs: ZOOKEEPER-1167, ZOOKEEPER-4747 (cherry picked from commit 3e2d6f3)

kezhuw · 2024-09-21T08:58:38Z

Backported to branch-3.9 since it is a simple handy wrapper for asynchronous sync. Also, it touches no data part.

cc @anmolnar

kezhuw force-pushed the ZOOKEEPER-4747-synchronous-sync branch from f1424cd to 689d70d Compare September 26, 2023 16:58

ctubbsii reviewed Sep 26, 2023

View reviewed changes

kezhuw mentioned this pull request Sep 27, 2023

ZOOKEEPER-3594: Don't propose error transactions #2070

Open

ztzg force-pushed the master branch from 1c60545 to e2070be Compare October 3, 2023 12:57

eolivelli approved these changes Nov 15, 2023

View reviewed changes

kezhuw mentioned this pull request May 29, 2024

ZOOKEEPER-4749: Respect zookeeper.request.timeout also for asynchronous api #2168

Open

kezhuw merged commit 3e2d6f3 into apache:master Sep 10, 2024

kezhuw deleted the ZOOKEEPER-4747-synchronous-sync branch October 14, 2024 02:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains #2068

ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains #2068

kezhuw commented Sep 26, 2023

ctubbsii Sep 26, 2023

kezhuw Sep 27, 2023 •

edited

Loading

ctubbsii Sep 29, 2023

ctubbsii Sep 26, 2023

kezhuw Sep 27, 2023

ctubbsii Sep 26, 2023

kezhuw Sep 27, 2023

eolivelli left a comment

kezhuw commented Aug 26, 2024

anmolnar commented Sep 18, 2024

kezhuw commented Sep 20, 2024

anmolnar commented Sep 20, 2024

kezhuw commented Sep 21, 2024

ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains #2068

ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains #2068

Conversation

kezhuw commented Sep 26, 2023

ctubbsii Sep 26, 2023

Choose a reason for hiding this comment

kezhuw Sep 27, 2023 • edited Loading

Choose a reason for hiding this comment

ctubbsii Sep 29, 2023

Choose a reason for hiding this comment

ctubbsii Sep 26, 2023

Choose a reason for hiding this comment

kezhuw Sep 27, 2023

Choose a reason for hiding this comment

ctubbsii Sep 26, 2023

Choose a reason for hiding this comment

kezhuw Sep 27, 2023

Choose a reason for hiding this comment

eolivelli left a comment

Choose a reason for hiding this comment

kezhuw commented Aug 26, 2024

anmolnar commented Sep 18, 2024

kezhuw commented Sep 20, 2024

anmolnar commented Sep 20, 2024

kezhuw commented Sep 21, 2024

kezhuw Sep 27, 2023 •

edited

Loading