You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "ctubbsii (via GitHub)" <gi...@apache.org> on 2023/08/24 16:44:17 UTC
[GitHub] [accumulo] ctubbsii opened a new pull request, #3720: Always use retry backoff in ZooReader(Writer)
ctubbsii opened a new pull request, #3720:
URL: https://github.com/apache/accumulo/pull/3720
Always use the retry backoff when always retrying. This fixes an issue where an "always retry" condition is triggered, it will not barrage ZooKeeper with repeated immediate retries.
This fixes #3718
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304690285
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
> Would it make sense to add anything about that exception to the message?
I can do that, but since the retry condition is passed in as a parameter, it won't be clear what about the exception triggered the retry, without also injecting a parameter to customize the log message for the given condition... which I think would complicate the method's API too much.
> Like how long we are waiting each time? Also can I presume that there is a cap as to the longest amount of period we will wait?
The wait time is logged at debug by the Retry class already. We don't log the max wait time or the backoff or other configuration, but it's hard-coded in the ZooReader class. We could log it, but I'm not sure where the best place to do that is where it would be useful.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii merged pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii merged PR #3720:
URL: https://github.com/apache/accumulo/pull/3720
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304613468
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
Review Comment:
Looking at RETRY_FACTORY at the top of the class it constructs a retry with `maxRetries(10)`. Would this mean a different retry factory is needed for the always retry case?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#issuecomment-1692291320
There was some discussion about changing the default maxWait interval being something longer than 5 seconds. I'm not so sure about that, but if that changes, it can be done in a separate 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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304629580
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
Would it make sense to add anything about that exception to the message?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#issuecomment-1692364046
Created #3723 to increase the timeout.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ivakegg commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ivakegg (via GitHub)" <gi...@apache.org>.
ivakegg commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304661373
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
Like how long we are waiting each time?
Also can I presume that there is a cap as to the longest amount of period we will wait?
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304699605
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
> Like how long we are waiting each time? Also can I presume that there is a cap as to the longest amount of period we will wait?
Can find the config for the retry here :
https://github.com/apache/accumulo/blob/cd7642988bac831292e988326aaa96e07938c641/core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java#L45
Looks like max wait time is 5 seconds. Wondering if that should be higher, like 30 seconds to a minute. If collisions keep happening, backing off more is the best thing to do.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304732386
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
I added the exception message to the log in 023415ae75728f90dc56dd0f6f9b0c75a157a68a
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] keith-turner commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304695404
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
> which I think would complicate the method's API too much.
Agreed. Wondering if appending the message from the exception would be useful, but was not sure.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
+ "attempting to communicate with zookeeper after exception that requires retry");
Review Comment:
> which I think would complicate the method's API too much.
Agreed. Wondering if appending the message from the exception would be useful, but not 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.
To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3720: Always use retry backoff in ZooReader(Writer)
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #3720:
URL: https://github.com/apache/accumulo/pull/3720#discussion_r1304624215
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooReader.java:
##########
@@ -184,7 +184,11 @@ protected <R> R retryLoopMutator(ZKFunctionMutator<R> zkf,
try {
return zkf.apply(getZooKeeper());
} catch (KeeperException e) {
- if (alwaysRetryCondition.test(e) || useRetryForTransient(retries, e)) {
+ if (alwaysRetryCondition.test(e)) {
+ retries.waitForNextAttempt(log,
Review Comment:
The maxRetries sets the counter. Calling the waitForNextAttempt does not decrement the counter. You have to call. `retries.useRetry()` to decrement the counter. This patch waits for the next retry using the backoff, but does not call `retries.useRetry()`.
--
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@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org