You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ratis.apache.org by "szetszwo (via GitHub)" <gi...@apache.org> on 2024/01/22 10:32:54 UTC
[PR] RATIS-2012. Client should not retry after close. [ratis]
szetszwo opened a new pull request, #1025:
URL: https://github.com/apache/ratis/pull/1025
See RATIS-2012
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1462104295
##########
ratis-test/src/test/java/org/apache/ratis/RaftLogTruncateTests.java:
##########
Review Comment:
Thanks for the clarification.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1463643042
##########
ratis-client/src/main/java/org/apache/ratis/client/impl/RaftClientImpl.java:
##########
@@ -105,7 +105,12 @@ public int getAttemptCount() {
}
public ClientRetryEvent newClientRetryEvent(RaftClientRequest request, Throwable throwable) {
+ return newClientRetryEvent(request, throwable, false);
+ }
+
+ ClientRetryEvent newClientRetryEvent(RaftClientRequest request, Throwable throwable, boolean get) {
Review Comment:
Is there a plan to call this with `get=true` somewhere? I didn't see it in current 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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on PR #1025:
URL: https://github.com/apache/ratis/pull/1025#issuecomment-1904888104
I tried this with RATIS-2013, but `testExceptionRetryAttempts` is still failing with `expected:<6> but was:<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.
To unsubscribe, e-mail: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1463691495
##########
ratis-client/src/main/java/org/apache/ratis/client/impl/RaftClientImpl.java:
##########
@@ -105,7 +105,12 @@ public int getAttemptCount() {
}
public ClientRetryEvent newClientRetryEvent(RaftClientRequest request, Throwable throwable) {
+ return newClientRetryEvent(request, throwable, false);
+ }
+
+ ClientRetryEvent newClientRetryEvent(RaftClientRequest request, Throwable throwable, boolean get) {
Review Comment:
You are right -- `get` is always false after refactoring. Let me combine the `newClientRetryEvent` methods.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1025:
URL: https://github.com/apache/ratis/pull/1025#issuecomment-1904388220
> ... but there is a [test failure](https://github.com/apache/ratis/actions/runs/7609811295/job/20721813016?pr=1025#step:6:746) in `TestExceptionDependentRetry.testExceptionRetryAttempts`
The tests also fail in the master branch in my machine. Filed RATIS-2013 to fix it separately.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "adoroszlai (via GitHub)" <gi...@apache.org>.
adoroszlai commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1461737506
##########
ratis-test/src/test/java/org/apache/ratis/RaftLogTruncateTests.java:
##########
Review Comment:
Doesn't this belong to RATIS-2011?
##########
ratis-client/src/main/java/org/apache/ratis/client/impl/OrderedAsync.java:
##########
@@ -199,30 +195,26 @@ private void sendRequestWithRetry(PendingOrderedRequest pending) {
return;
}
- final RetryPolicy retryPolicy = client.getRetryPolicy();
sendRequest(pending).exceptionally(e -> {
if (e instanceof CompletionException) {
e = JavaUtils.unwrapCompletionException(e);
- scheduleWithTimeout(pending, request, retryPolicy, e);
+ scheduleWithTimeout(pending, request, e);
return null;
}
f.completeExceptionally(e);
return null;
});
}
- private void scheduleWithTimeout(PendingOrderedRequest pending,
- RaftClientRequest request, RetryPolicy retryPolicy, Throwable e) {
- final int attempt = pending.getAttemptCount();
- final ClientRetryEvent event = new ClientRetryEvent(request, e, pending);
+ private void scheduleWithTimeout(PendingOrderedRequest pending, RaftClientRequest request, Throwable e) {
+ final RetryPolicy retryPolicy = client.getRetryPolicy();
+ final ClientRetryEvent event = pending.newClientRetryEvent(request, e);
final TimeDuration sleepTime = client.getEffectiveSleepTime(e,
retryPolicy.handleAttemptFailure(event).getSleepTime());
- LOG.debug("schedule* attempt #{} with sleep {} and policy {} for {}", attempt, sleepTime, retryPolicy, request);
- scheduleWithTimeout(pending, sleepTime, getSlidingWindow(request));
- }
+ LOG.debug("schedule* attempt #{} with sleep {} and policy {} for {}",
+ pending.getAttemptCount(), sleepTime, retryPolicy, request);
Review Comment:
nit: get attempt count from `event` (where it's `final`) instead of `pending`
```suggestion
event.getAttemptCount(), sleepTime, retryPolicy, request);
```
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1025:
URL: https://github.com/apache/ratis/pull/1025#issuecomment-1905116625
> failing with `expected:<6> but was:<4>`.
`OrderedAsync` calls `newClientRetryEvent` twice so it increments the exception count per retry.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo merged PR #1025:
URL: https://github.com/apache/ratis/pull/1025
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on PR #1025:
URL: https://github.com/apache/ratis/pull/1025#issuecomment-1906623278
@adoroszlai , thanks a lot for reviewing 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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1462100729
##########
ratis-test/src/test/java/org/apache/ratis/RaftLogTruncateTests.java:
##########
Review Comment:
This is a basic trancate test. RATIS-2011 will add check if `TransactionContext` objects are removed correctly.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] RATIS-2012. Client should not retry after close. [ratis]
Posted by "szetszwo (via GitHub)" <gi...@apache.org>.
szetszwo commented on code in PR #1025:
URL: https://github.com/apache/ratis/pull/1025#discussion_r1462455727
##########
ratis-test/src/test/java/org/apache/ratis/RaftLogTruncateTests.java:
##########
Review Comment:
BTW, the test will fail without the fix here:
1. A client write a few `messagesToBeTruncated` requests to the old leader with three nodes (n3, n4, n5) down in a 5-node group. These entries are written to the log but they cannot be committed since only two nodes (n1, n2) are available.
2. Shut down n1, n2.
3. Restart n3, n4, n5. These nodes have a majority. Then can elect a new leader and commit new entires.
4. Restart n1, n2. They should truncate their log and the `messagesToBeTruncated` entries will disappear.
Bug: since the client keeps retrying even after it has been closed, the client will retry the same requests to the new leader. The `messagesToBeTruncated` entries will re-appear.
--
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: issues-unsubscribe@ratis.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org