[GitHub] [bookkeeper] StevenLuMT commented on a diff in pull request #3041: Issue #3005: Fix ack broken entry succeed in ensemble change unsetting

[GitBox <gi...@apache.org>]

StevenLuMT commented on code in PR #3041:
URL: https://github.com/apache/bookkeeper/pull/3041#discussion_r931063538


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java:
##########
@@ -2002,6 +2002,20 @@ void unsetSuccessAndSendWriteRequest(List<BookieId> ensemble, final Set<Integer>
                 pendingAddOp.unsetSuccessAndSendWriteRequest(ensemble, bookieIndex);
             }
         }
+        // Some entries could fulfill ack requirement before and after ensemble changed.
+        // We need to invoke #sendAddSuccessCallbacks() for such entries because
+        // they may have already completed, but they are just waiting for the ensemble
+        // change to complete.
+        // E.g.
+        // ensemble (A, B, C, D), entry k is written to (A, B, D). An ensemble change
+        // happens to replace C with E. Entry k does not complete until C is
+        // replaced with E successfully. When the ensemble change completes, it tries
+        // to unset entry k. C however is not in k's write set, so no entry is written
+        // again, and no one triggers #sendAddSuccessCallbacks. Consequently, k never
+        // completes.
+        //
+        // We call sendAddSuccessCallback after unsetting all pending adds to cover this case.
+        sendAddSuccessCallbacks();

Review Comment:
   from multiple pendingAddOps requests, just call sendAddSuccessCallbacks once time? 
   is it right？ @kezhuw 



-- 
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@bookkeeper.apache.org

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