Re: [PR] RATIS-2433. Cancel transaction in case of failure to append [ratis]

["szetszwo (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@ratis.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org