szetszwo commented on code in PR #1382:
URL: https://github.com/apache/ratis/pull/1382#discussion_r3023926514
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -838,52 +845,73 @@ private CompletableFuture<RaftClientReply>
appendTransaction(
final LeaderStateImpl unsyncedLeaderState =
role.getLeaderState().orElse(null);
if (unsyncedLeaderState == null) {
- final RaftClientReply reply = newExceptionReply(request,
generateNotLeaderException());
+ final NotLeaderException nle = generateNotLeaderException();
+ final RaftClientReply reply = newExceptionReply(request, nle);
+ cancelTransaction(context, nle);
return RetryCacheImpl.failWithReply(reply, cacheEntry);
Review Comment:
Let's move the failWithReply method to RaftServerImpl:
```java
private CompletableFuture<RaftClientReply> failWithReply(RaftClientReply
reply, CacheEntry entry,
TransactionContextImpl context) {
cancelTransaction(context, reply.getException());
if (entry == null) {
return CompletableFuture.completedFuture(reply);
}
entry.failWithReply(reply);
return entry.getReplyFuture();
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
Review Comment:
Please keep this method and call cancelTransaction inside. We should use it
for the other ResourceUnavailableException cases.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -838,52 +845,73 @@ private CompletableFuture<RaftClientReply>
appendTransaction(
final LeaderStateImpl unsyncedLeaderState =
role.getLeaderState().orElse(null);
if (unsyncedLeaderState == null) {
- final RaftClientReply reply = newExceptionReply(request,
generateNotLeaderException());
+ final NotLeaderException nle = generateNotLeaderException();
+ final RaftClientReply reply = newExceptionReply(request, nle);
+ cancelTransaction(context, nle);
return RetryCacheImpl.failWithReply(reply, cacheEntry);
}
final PendingRequests.Permit unsyncedPermit =
unsyncedLeaderState.tryAcquirePendingRequest(request.getMessage());
if (unsyncedPermit == null) {
- return getResourceUnavailableReply(request, cacheEntry);
+ final ResourceUnavailableException e = new ResourceUnavailableException(
+ getMemberId() + ": Failed to acquire a pending write request for " +
request);
+ cancelTransaction(context, e);
+ return cacheEntry.failWithException(e);
}
- final LeaderStateImpl leaderState;
- final PendingRequest pending;
+ LeaderStateImpl leaderState = null;
+ PendingRequest pending = null;
+ CompletableFuture<RaftClientReply> failure = null;
+ Exception cancelException = null;
Review Comment:
We should keep returning immediately in case of a failure. This change make
the code harder to read.
##########
ratis-server/src/test/java/org/apache/ratis/server/impl/RaftStateMachineExceptionTests.java:
##########
@@ -179,4 +187,31 @@ private void
runTestRetryOnExceptionDuringReplication(CLUSTER cluster) throws Ex
failPreAppend = false;
}
}
+
+ @Test
+ public void testCancelTransactionOnPreAppendFailure() throws Exception {
+ runWithNewCluster(3, this::runTestCancelTransactionOnPreAppendFailure);
+ }
+
+ private void runTestCancelTransactionOnPreAppendFailure(CLUSTER cluster)
throws Exception {
+ final RaftPeerId leaderId = RaftTestUtil.waitForLeader(cluster).getId();
+ failPreAppend = true;
+ numCancelTransaction.set(0);
+ try (final RaftClient client = cluster.createClient(leaderId)) {
+ try {
+ client.io().send(new SimpleMessage("cancel-transaction"));
+ fail("Exception expected");
+ } catch (StateMachineException e) {
+ Assertions.assertTrue(e.getCause().getMessage().contains("Fake
Exception in preAppend"));
+ }
+
+ JavaUtils.attemptRepeatedly(() -> {
+ Assertions.assertTrue(numCancelTransaction.get() > 0,
Review Comment:
Could we check the exact number instead of "> 0"?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
