You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2022/05/05 00:58:03 UTC
[GitHub] [helix] rabashizade opened a new pull request, #2082: Add TTL and Container Modes to ZkClient APIs
rabashizade opened a new pull request, #2082:
URL: https://github.com/apache/helix/pull/2082
This PR adds the TTL and Container node creation capability to ZkClient APIs.
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #2081
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
This PR adds methods that support creating container nodes and persistent nodes with TTL to several classes, including ZkConnection, ZkClient and its descendants in zookeeper-api, and BaseDataAccessors in helix-core.
### Tests
- [ ] The following tests are written for this issue:
The changes have been tested locally, but since Zookeeper server makes no guarantee on exactly when the Container and TTL nodes are deleted (see [here](https://zookeeper.apache.org/doc/r3.5.9/zookeeperProgrammers.html#TTL+Nodes)), the tests become flaky and as a result, are not added to this PR.
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870559369
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
Can we get rid of this `doAsyncCreate()` and just use the one with `ttl = -1`? This will help us reduce duplicate code.
That's what the upstream ZK is doing anyway - see [here](https://github.com/apache/zookeeper/blob/724864487457d6f2eb7c331ef338936e1375072b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L1561)
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870541234
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
It seems that this is just a formatting/style change. Am I missing anything 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r869643782
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java:
##########
@@ -111,12 +112,17 @@ protected void recordFailure(int rc, String path, ZkAsyncCallMonitorContext moni
}
}
- public static class CreateCallbackHandler extends DefaultCallback implements StringCallback {
+ public static class CreateCallbackHandler extends DefaultCallback implements StringCallback, Create2Callback {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
callback(rc, path, ctx);
}
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name, Stat stat) {
Review Comment:
It's a Zookeeper data structure used for returning metadata about a ZNode. This particular method is part of the `Create2Callback` interface that has to be implemented.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870559369
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
Can we get rid of this `doAsyncCreate()` and just use the one with `ttl = -1`? That's what the upstream ZK is doing anyway...
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r865964821
##########
helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java:
##########
@@ -43,6 +43,18 @@
*/
boolean create(String path, T record, int options);
+ /**
Review Comment:
General comment:
Good work. This is a very large PR - I think instead of trying to include all changes in one single PR, it would help you and the reviewers to split this up. This will help us:
- Enable modular development
- Facilitate review process (faster turnaround)
- Better quality of reviews
One way I could think of off the top of my head is to first focus on the classes at the lowest level (e.g. ZkConnection, AsyncCallback, etc.) since we need to get those right first. Then move up a layer (ZkClient). And then another layer - (FederatedZkClient, DedicatedZkClient, etc. - these are all similar, so once we get one right, we probably get the rest right). And next, cover BaseDataAccessor and the like.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r869454573
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java:
##########
@@ -111,12 +112,17 @@ protected void recordFailure(int rc, String path, ZkAsyncCallMonitorContext moni
}
}
- public static class CreateCallbackHandler extends DefaultCallback implements StringCallback {
+ public static class CreateCallbackHandler extends DefaultCallback implements StringCallback, Create2Callback {
Review Comment:
The reason is the callback type in Zookeeper which supports TTL mode is of type `Create2Callback` (see [here](https://github.com/apache/zookeeper/blob/master/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L1602)). So I either had to add another `CallbackHandler` type that implements `Create2Callback`, or add `Create2Callback` to `CreateCallbackHandler` to support async create operations with TTL support. Adding a new type resulted in more duplicate code of the same methods with different argument types downstream, so I decided to just have `CreateCallbackHandler` implement `Create2Callback` instead.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870559369
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
Can we get rid of this `doAsyncCreate()` and just use the one with `ttl = -1`?
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r865956473
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java:
##########
@@ -61,6 +61,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.helix.zookeeper.zkclient.ZkClient.DEFAULT_TTL;
Review Comment:
Usually we try to avoid static imports. ZkClient.DEFAULT_TTL should work?
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r869456442
##########
helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java:
##########
@@ -43,6 +43,18 @@
*/
boolean create(String path, T record, int options);
+ /**
Review Comment:
Thank you for the suggestion. I broke it up into multiple PRs. In this one, I just add TTL mode to async create API. In the next, I add it to sync API in `ZkClient` and `ZkConnection`, then to `RealmAwareZkClient` and its descendants, and finally to `BaseDataAcessors`.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r869450611
##########
helix-core/src/main/java/org/apache/helix/manager/zk/ZkBaseDataAccessor.java:
##########
@@ -61,6 +61,7 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static org.apache.helix.zookeeper.zkclient.ZkClient.DEFAULT_TTL;
Review Comment:
Thank you for the suggestion. Since I ended up breaking up the PR into multiple smaller ones, this variable doesn't show up in this PR anymore. But I'll import it the way you suggested in the next PRs.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
rabashizade commented on PR #2082:
URL: https://github.com/apache/helix/pull/2082#issuecomment-1122821548
> For adding TTL, don't you need to add some tests?
This is a valid concern. However, as I explained in the PR description, the changes have been tested locally but since Zookeeper server makes no guarantee on exactly when the TTL nodes are deleted (see [here](https://zookeeper.apache.org/doc/r3.5.9/zookeeperProgrammers.html#TTL+Nodes)), the tests become flaky and as a result, are not added to this PR. I can add the tests, but we either need to wait a long time to be sure nodes are deleted, or we will see random test failures in the CI workflow.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
rabashizade commented on PR #2082:
URL: https://github.com/apache/helix/pull/2082#issuecomment-1124100545
This PR is ready to be merged, approved by @narendly.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870657298
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -85,6 +86,7 @@
public class ZkClient implements Watcher {
private static final Logger LOG = LoggerFactory.getLogger(ZkClient.class);
+ public static final long DEFAULT_TTL = -1L;
Review Comment:
Nit: Would `TTL_NOT_SET` be a more descriptive name for this? Also, I see this is a `public` constant - are you planning to use this in other classes?
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r865960611
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java:
##########
@@ -111,12 +112,17 @@ protected void recordFailure(int rc, String path, ZkAsyncCallMonitorContext moni
}
}
- public static class CreateCallbackHandler extends DefaultCallback implements StringCallback {
+ public static class CreateCallbackHandler extends DefaultCallback implements StringCallback, Create2Callback {
Review Comment:
Could you help us understand why this change is required?
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870660486
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -85,6 +86,7 @@
public class ZkClient implements Watcher {
private static final Logger LOG = LoggerFactory.getLogger(ZkClient.class);
+ public static final long DEFAULT_TTL = -1L;
Review Comment:
Yes, that's a better name. I'll change it.
Also yes, it will be used in other classes such as BaseDataAccessors and ZkClient implementations. I was thinking of creating a separate class for this constant, but since it was only one variable I thought I'd just put it in ZkClient. Let me know if a separate constants class is preferred and I can add 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] rabashizade commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
rabashizade commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870601169
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
Thank you for the suggestion, it makes sense and I made the change.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870541234
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
It seems that this is just a formatting/style change. Am I missing anything 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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL and Container Modes to ZkClient APIs
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r865955715
##########
helix-core/src/main/java/org/apache/helix/BaseDataAccessor.java:
##########
@@ -95,6 +107,17 @@
*/
boolean[] createChildren(List<String> paths, List<T> records, int options);
+ /**
+ * Use it when creating children under a parent node. This will use async api for better
+ * performance. If the child already exists it will return false.
Review Comment:
the child (single) or children (all of them)?
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870552261
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java:
##########
@@ -111,12 +112,17 @@ protected void recordFailure(int rc, String path, ZkAsyncCallMonitorContext moni
}
}
- public static class CreateCallbackHandler extends DefaultCallback implements StringCallback {
+ public static class CreateCallbackHandler extends DefaultCallback implements StringCallback, Create2Callback {
Review Comment:
Makes sense. I was expecting to see a new `Create2CallbackHandler`, but since we're not doing much other than recording some stats in the monitor, I'm fine with this.
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] desaikomal commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
desaikomal commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r869627643
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java:
##########
@@ -111,12 +112,17 @@ protected void recordFailure(int rc, String path, ZkAsyncCallMonitorContext moni
}
}
- public static class CreateCallbackHandler extends DefaultCallback implements StringCallback {
+ public static class CreateCallbackHandler extends DefaultCallback implements StringCallback, Create2Callback {
@Override
public void processResult(int rc, String path, Object ctx, String name) {
callback(rc, path, ctx);
}
+ @Override
+ public void processResult(int rc, String path, Object ctx, String name, Stat stat) {
Review Comment:
what is 'stat' object used for??
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly commented on a diff in pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly commented on code in PR #2082:
URL: https://github.com/apache/helix/pull/2082#discussion_r870559369
##########
zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java:
##########
@@ -1942,22 +1943,61 @@ public void asyncCreate(final String path, Object datat, final CreateMode mode,
private void doAsyncCreate(final String path, final byte[] data, final CreateMode mode,
final long startT, final ZkAsyncCallbacks.CreateCallbackHandler cb, final String expectedSessionId) {
+ try {
+ retryUntilConnected(() -> {
+ getExpectedZookeeper(expectedSessionId)
+ .create(path, data, ZooDefs.Ids.OPEN_ACL_UNSAFE, mode,
+ (AsyncCallback.StringCallback) cb,
+ new ZkAsyncRetryCallContext(_asyncCallRetryThread, cb, _monitor, startT, 0, false,
+ GZipCompressionUtil.isCompressed(data)) {
+ @Override
+ protected void doRetry() {
+ doAsyncCreate(path, data, mode, System.currentTimeMillis(), cb,
+ expectedSessionId);
+ }
+ });
+ return null;
+ });
+ } catch (RuntimeException e) {
+ // Process callback to release caller from waiting
+ cb.processResult(KeeperException.Code.APIERROR.intValue(), path,
+ new ZkAsyncCallMonitorContext(_monitor, startT, 0, false), null);
+ throw e;
+ }
+ }
Review Comment:
Can we get rid of this `doAsyncCreate()` and just use the one with `ttl` parameter with its value set to `-1`? This will help us reduce duplicate code.
That's what the upstream ZK is doing anyway - see [here](https://github.com/apache/zookeeper/blob/724864487457d6f2eb7c331ef338936e1375072b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java#L1561)
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] narendly merged pull request #2082: Add TTL mode to async create API in ZkClient
Posted by GitBox <gi...@apache.org>.
narendly merged PR #2082:
URL: https://github.com/apache/helix/pull/2082
--
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: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org