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 2021/04/28 17:19:18 UTC
[GitHub] [helix] rabashizade opened a new pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
rabashizade opened a new pull request #1718:
URL: https://github.com/apache/helix/pull/1718
### Issues
- [x] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1717
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
When there is a ZK session mismatch for async operations in ZkClient, passing UNKNOWN_RET_CODE to the appropriate ZkAsyncCallback and not checking for this condition in the needRetry() method in the ZkAsyncCallback results in throwing a NullPointerException which can be confusing. This PR defines an additional return code for the ZK session mismatch situation and by checking for this condition in the needRetry() method, provides a more meaningful message in the log.
### Tests
- [x] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
helix-core:
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] TestExpandCluster.testClusterExpansion:61 expected:<false> but was:<true>
[INFO]
[ERROR] Tests run: 1265, Failures: 1, Errors: 0, Skipped: 2
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:56 h
[INFO] Finished at: 2021-04-28T09:57:01-07:00
[INFO] ------------------------------------------------------------------------
```
TestExpandCluster:
```
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 94.638 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco-maven-plugin:0.8.6:report (generate-code-coverage-report) @ helix-core ---
[INFO] Loading execution data file /Users/rabashiz/IdeaProjects/helix/helix-core/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: Core' with 890 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:41 min
[INFO] Finished at: 2021-04-28T10:01:06-07:00
[INFO] ------------------------------------------------------------------------
```
zookeeper-api:
```
[INFO] Results:
[INFO]
[INFO] Tests run: 54, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO]
[INFO] --- jacoco-maven-plugin:0.8.6:report (generate-code-coverage-report) @ zookeeper-api ---
[INFO] Loading execution data file /Users/rabashiz/IdeaProjects/helix/zookeeper-api/target/jacoco.exec
[INFO] Analyzed bundle 'Apache Helix :: ZooKeeper API' with 115 classes
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 09:18 min
[INFO] Finished at: 2021-04-28T10:16:51-07:00
[INFO] ------------------------------------------------------------------------
```
### 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.
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] jiajunwang merged pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
jiajunwang merged pull request #1718:
URL: https://github.com/apache/helix/pull/1718
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622416789
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
Yes, sort of. I noticed that KeeperException.Code int values are all negative, so I chose a positive number (inspired by the choice of 255 for UNKNOWN_RET_CODE) to avoid any unwanted collision with those. I don't what the best practice is for choosing return codes, please let me know if I should change it to a more meaningful value.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622410325
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
Review comment:
Nit: format
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622416789
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
Yes, sort of. I noticed that KeeperException.Code int values are all negative, so I chose a positive number (inspired by the choice of 255 for UNKNOWN_RET_CODE) to avoid any unwanted collision with those. I don't know what the best practice is for choosing return codes, please let me know if I should change it to a more meaningful value.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622649415
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
Thanks for the resources. I'll make sure there's no collision with these constant values, and will also add appropriate comments to the code.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622452591
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
Also, it might be a good idea to check https://zookeeper.apache.org/doc/r3.6.2/apidocs/zookeeper-server/constant-values.html to avoid collision.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r625481954
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
Review comment:
Thanks for the comment, I moved it before the switch block. For the moment I haven't defined a new class, but if I do so I will use that class in the switch block.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622452399
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
`ZkAsyncCallbacks` is a `zookeeper-api` module's code, so essentially it doesn't really matter and we don't have a set convention for what value we need to use.
I'd suggest that we add a block of comment here describing the situation where we should throw this exception code? For whoever gets to debug this using the code or for future developers so they know whether using this code would be appropriate.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622454987
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
+ LOG.error("Actual session ID doesn't match with expected session ID. Skip retrying.");
+ } else {
+ LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ }
Review comment:
This code seems just one-off and doesn't look to extendable. Do you think this would be a good opportunity to move all Helix Project's `zookeeper-api` error codes into one class so it's easier to track?
For example, see the example in `KeeperException` (this is part of the ZooKeeper project):
```
@Public
public static enum Code implements KeeperException.CodeDeprecated {
OK(0),
SYSTEMERROR(-1),
RUNTIMEINCONSISTENCY(-2),
DATAINCONSISTENCY(-3),
CONNECTIONLOSS(-4),
MARSHALLINGERROR(-5),
UNIMPLEMENTED(-6),
OPERATIONTIMEOUT(-7),
BADARGUMENTS(-8),
APIERROR(-100),
NONODE(-101),
NOAUTH(-102),
BADVERSION(-103),
NOCHILDRENFOREPHEMERALS(-108),
NODEEXISTS(-110),
NOTEMPTY(-111),
SESSIONEXPIRED(-112),
INVALIDCALLBACK(-113),
INVALIDACL(-114),
AUTHFAILED(-115),
SESSIONMOVED(-118),
NOTREADONLY(-119);
```
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622416018
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
Review comment:
Thanks, I'll fix it in the next commit.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622454987
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
+ LOG.error("Actual session ID doesn't match with expected session ID. Skip retrying.");
+ } else {
+ LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ }
Review comment:
This code seems just one-off and doesn't look extendable. Do you think this would be a good opportunity to move all Helix Project's `zookeeper-api` error codes into one class so it's easier to track?
For example, see the example in `KeeperException` (this is part of the ZooKeeper project):
```
@Public
public static enum Code implements KeeperException.CodeDeprecated {
OK(0),
SYSTEMERROR(-1),
RUNTIMEINCONSISTENCY(-2),
DATAINCONSISTENCY(-3),
CONNECTIONLOSS(-4),
MARSHALLINGERROR(-5),
UNIMPLEMENTED(-6),
OPERATIONTIMEOUT(-7),
BADARGUMENTS(-8),
APIERROR(-100),
NONODE(-101),
NOAUTH(-102),
BADVERSION(-103),
NOCHILDRENFOREPHEMERALS(-108),
NODEEXISTS(-110),
NOTEMPTY(-111),
SESSIONEXPIRED(-112),
INVALIDCALLBACK(-113),
INVALIDACL(-114),
AUTHFAILED(-115),
SESSIONMOVED(-118),
NOTREADONLY(-119);
```
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r625481698
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
+ LOG.error("Actual session ID doesn't match with expected session ID. Skip retrying.");
+ } else {
+ LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ }
Review comment:
Based on the different exception classes that extend `ZkException` in the package `org.apache.helix.zookeeper.zkclient.exception`, only `ZkSessionMismatchedException` and `ZkInterruptedException` don't already have corresponding codes in `KeeperException.Code`, and `ZkInterruptedException` seems to be only adding thread interruption capability to `ZkException`. So it seems to me that `ZkSessionMismatchedException` is the only exception type in Helix project without a corresponding `KeeperException.Code`. Do we expect more error codes to be added in the future? Because otherwise, it seems to me that adding a new class for this one error code is a bit of an overkill. But if you think this is the right approach, I will add the class.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r625482110
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1962,8 +1962,10 @@ protected void doRetry() {
});
} catch (RuntimeException e) {
Review comment:
Thanks, it is easier to read the way you suggested and I fixed 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.
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 #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on pull request #1718:
URL: https://github.com/apache/helix/pull/1718#issuecomment-832821131
This PR is ready to be merged, approved by @jiajunwang.
**Commit message:**
Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Change the error message for ZkSessionMismatchedException in ZkAsyncCallbacks to be clearer instead of throwing a NullPointerException.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
narendly commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622410892
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -36,6 +36,7 @@
public class ZkAsyncCallbacks {
private static Logger LOG = LoggerFactory.getLogger(ZkAsyncCallbacks.class);
public static final int UNKNOWN_RET_CODE = 255;
+ public static final int ZK_SESSION_MISMATCHED_CODE = 127;
Review comment:
Is 127 an arbitrary number?
--
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.
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] jiajunwang commented on a change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r623402294
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
Review comment:
This should not fall into Exception handling logic, it is the expected RC number.
So if we are moving all the return code into a new class, then the switch case should check using that new class, and this is no longer a special case.
If, otherwise, we are not using a new class then, we can check this condition before the switch block.
--
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.
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 change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
rabashizade commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r622649931
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/callback/ZkAsyncCallbacks.java
##########
@@ -289,7 +290,11 @@ protected boolean needRetry(int rc) {
return false;
}
} catch (ClassCastException | NullPointerException ex) {
- LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ if(rc == ZK_SESSION_MISMATCHED_CODE) {
+ LOG.error("Actual session ID doesn't match with expected session ID. Skip retrying.");
+ } else {
+ LOG.error("Failed to handle unknown return code {}. Skip retrying.", rc, ex);
+ }
Review comment:
This makes sense, I'll move all of the error codes into one class.
--
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.
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] jiajunwang commented on a change in pull request #1718: Clarify error for ZkSessionMismatchedException in ZkAsyncCallbacks
Posted by GitBox <gi...@apache.org>.
jiajunwang commented on a change in pull request #1718:
URL: https://github.com/apache/helix/pull/1718#discussion_r623404160
##########
File path: zookeeper-api/src/main/java/org/apache/helix/zookeeper/zkclient/ZkClient.java
##########
@@ -1962,8 +1962,10 @@ protected void doRetry() {
});
} catch (RuntimeException e) {
Review comment:
nit, I slightly prefer the following alternative.
} catch (ZkSessionMismatchedException e) {
...
} catch (RuntimeException e) {
...
}
--
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.
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