You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by "kezhuw (via GitHub)" <gi...@apache.org> on 2023/09/26 16:52:14 UTC
[GitHub] [zookeeper] kezhuw opened a new pull request, #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
kezhuw opened a new pull request, #2068:
URL: https://github.com/apache/zookeeper/pull/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
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#issuecomment-2364339090
Yes, we only have [backward compatiblity](https://cwiki.apache.org/confluence/display/ZOOKEEPER/ReleaseManagement) rules, but not forward compatibility.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1338249819
##########
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:
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`.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#issuecomment-2365067996
Backported to `branch-3.9` since it is a simple handy wrapper for asynchronous `sync`. Also, it touches no data part.
cc @anmolnar
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#issuecomment-2309390321
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(#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.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "anmolnar (via GitHub)" <gi...@apache.org>.
anmolnar commented on PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#issuecomment-2359368165
@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.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#issuecomment-2363626056
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 (https://github.com/apache/zookeeper/pull/1917#discussion_r1766935909). :-)
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1338251206
##########
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:
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.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1338249819
##########
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:
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 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`.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] kezhuw commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1338263432
##########
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:
> 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.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ctubbsii commented on a diff in pull request #2068: ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068#discussion_r1340830026
##########
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:
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.
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] ZOOKEEPER-4747: Add synchronous sync to ease synchronous call chains [zookeeper]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw merged PR #2068:
URL: https://github.com/apache/zookeeper/pull/2068
--
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@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org