You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@curator.apache.org by GitBox <gi...@apache.org> on 2020/05/04 05:30:51 UTC
[GitHub] [curator] TisonKun opened a new pull request #363: CURATOR-544: SessionFailedRetryPolicy
TisonKun opened a new pull request #363:
URL: https://github.com/apache/curator/pull/363
More details on [JIRA](https://issues.apache.org/jira/browse/CURATOR-544#).
The breaking parts are we remove `RetryLoop.shouldRetry` & `RetryLoop. isRetryException` but it is no harm to add them back if we want to keep public APIs.
cc @Randgalt @shayshim
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #363:
URL: https://github.com/apache/curator/pull/363#discussion_r421174839
##########
File path: curator-framework/src/main/java/org/apache/curator/framework/imps/CuratorFrameworkImpl.java
##########
@@ -637,7 +637,8 @@ boolean useContainerParentsIfAvailable()
boolean doQueueOperation = false;
do
{
- if ( RetryLoop.shouldRetry(event.getResultCode()) )
+ final KeeperException ke = KeeperException.create(event.getResultCode());
Review comment:
nit - for me creating `ke` is unneeded. Create the exception directly as the argument
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #363:
URL: https://github.com/apache/curator/pull/363#issuecomment-625346195
Updated
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
Randgalt commented on pull request #363:
URL: https://github.com/apache/curator/pull/363#issuecomment-624964838
Please add details on the breaking changes to https://github.com/apache/curator/blob/master/src/site/confluence/breaking-changes.confluence
----------------------------------------------------------------
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
[GitHub] [curator] Randgalt commented on a change in pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
Randgalt commented on a change in pull request #363:
URL: https://github.com/apache/curator/pull/363#discussion_r421203375
##########
File path: curator-client/src/main/java/org/apache/curator/RetryPolicy.java
##########
@@ -33,5 +35,26 @@
* @param sleeper use this to sleep - DO NOT call Thread.sleep
* @return true/false
*/
- public boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+ boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+
+ /**
+ * Called when an operation has failed with a specific exception. This method
+ * should return true to make another attempt.
+ *
+ * @param exception the cause that this operation failed
+ * @return true/false
+ */
+ default boolean allowRetry(Throwable exception)
+ {
+ if ( exception instanceof KeeperException)
+ {
+ final int rc = ((KeeperException) exception).code().intValue();
+ return (rc == KeeperException.Code.CONNECTIONLOSS.intValue()) ||
+ (rc == KeeperException.Code.OPERATIONTIMEOUT.intValue()) ||
+ (rc == KeeperException.Code.SESSIONMOVED.intValue()) ||
+ (rc == KeeperException.Code.SESSIONEXPIRED.intValue()) ||
+ (rc == -13); // KeeperException.Code.NEWCONFIGNOQUORUM.intValue()) - using hard coded value for ZK 3.4.x compatibility
Review comment:
Yes for sure
----------------------------------------------------------------
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
[GitHub] [curator] cammckenzie commented on a change in pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
cammckenzie commented on a change in pull request #363:
URL: https://github.com/apache/curator/pull/363#discussion_r421202570
##########
File path: curator-client/src/main/java/org/apache/curator/RetryPolicy.java
##########
@@ -33,5 +35,26 @@
* @param sleeper use this to sleep - DO NOT call Thread.sleep
* @return true/false
*/
- public boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+ boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+
+ /**
+ * Called when an operation has failed with a specific exception. This method
+ * should return true to make another attempt.
+ *
+ * @param exception the cause that this operation failed
+ * @return true/false
+ */
+ default boolean allowRetry(Throwable exception)
+ {
+ if ( exception instanceof KeeperException)
+ {
+ final int rc = ((KeeperException) exception).code().intValue();
+ return (rc == KeeperException.Code.CONNECTIONLOSS.intValue()) ||
+ (rc == KeeperException.Code.OPERATIONTIMEOUT.intValue()) ||
+ (rc == KeeperException.Code.SESSIONMOVED.intValue()) ||
+ (rc == KeeperException.Code.SESSIONEXPIRED.intValue()) ||
+ (rc == -13); // KeeperException.Code.NEWCONFIGNOQUORUM.intValue()) - using hard coded value for ZK 3.4.x compatibility
Review comment:
@Randgalt Can we move this to a constant now that we're no longer supporting 3.4?
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #363:
URL: https://github.com/apache/curator/pull/363#issuecomment-625211910
Thanks for your reviews! Will update in hours.
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
TisonKun commented on pull request #363:
URL: https://github.com/apache/curator/pull/363#issuecomment-625677031
Thanks!
----------------------------------------------------------------
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
[GitHub] [curator] TisonKun commented on a change in pull request #363: CURATOR-544: SessionFailedRetryPolicy
Posted by GitBox <gi...@apache.org>.
TisonKun commented on a change in pull request #363:
URL: https://github.com/apache/curator/pull/363#discussion_r421449573
##########
File path: curator-client/src/main/java/org/apache/curator/RetryPolicy.java
##########
@@ -33,5 +35,26 @@
* @param sleeper use this to sleep - DO NOT call Thread.sleep
* @return true/false
*/
- public boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+ boolean allowRetry(int retryCount, long elapsedTimeMs, RetrySleeper sleeper);
+
+ /**
+ * Called when an operation has failed with a specific exception. This method
+ * should return true to make another attempt.
+ *
+ * @param exception the cause that this operation failed
+ * @return true/false
+ */
+ default boolean allowRetry(Throwable exception)
+ {
+ if ( exception instanceof KeeperException)
+ {
+ final int rc = ((KeeperException) exception).code().intValue();
+ return (rc == KeeperException.Code.CONNECTIONLOSS.intValue()) ||
+ (rc == KeeperException.Code.OPERATIONTIMEOUT.intValue()) ||
+ (rc == KeeperException.Code.SESSIONMOVED.intValue()) ||
+ (rc == KeeperException.Code.SESSIONEXPIRED.intValue()) ||
+ (rc == -13); // KeeperException.Code.NEWCONFIGNOQUORUM.intValue()) - using hard coded value for ZK 3.4.x compatibility
Review comment:
Thanks for spotting this. Will update.
----------------------------------------------------------------
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