[GitHub] [bookkeeper] fpj commented on a change in pull request #2654: Issue 2615: Fix for invalid ensemble issue during ledger recovery

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

fpj commented on a change in pull request #2654:
URL: https://github.com/apache/bookkeeper/pull/2654#discussion_r610403102



##########
File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java
##########
@@ -104,7 +104,24 @@ LedgerRecoveryOp setEntryListener(ReadEntryListener entryListener) {
                     public void readLastConfirmedDataComplete(int rc, RecoveryData data) {
                         if (rc == BKException.Code.OK) {
                             synchronized (lh) {
-                                lh.lastAddPushed = lh.lastAddConfirmed = data.getLastAddConfirmed();
+                                /**
+                                 The lowest an LAC can be for use in recovery is the first entry id
+                                 of the current ensemble - 1.
+                                 All ensembles prior to the current one, if any, are confirmed and
+                                 immutable (so are not part of the recovery process).
+                                 So we take the highest of:
+                                 - the LAC returned by the current bookie ensemble (could be -1)
+                                 - the first entry id of the current ensemble - 1.
+                                 */
+                                Long lastEnsembleEntryId = lh.getVersionedLedgerMetadata()
+                                        .getValue()
+                                        .getAllEnsembles()
+                                        .lastEntry()
+                                        .getKey();
+
+                                lh.lastAddPushed = lh.lastAddConfirmed = Math.max(data.getLastAddConfirmed(),

Review comment:
       Thanks for the patch, @Vanlightly . I understand that you are trying to lower bound the value of last add pushed by getting the last entry id from the last ensemble in the ledger history. I'd appreciate if you could clarify a couple of quick things for me:
   
   1- Why is it `lastEnsembleEntryId - 1` and not `lastEnsembleEntryId`? I'm missing why we need the `-1`.
   2- The PR description mentions the following:
   
   >  The main issue is that if an ensemble change occurs when writing back entries, it will try and create a new ensemble with first entry id of 0. This causes an IllegalStateException as there is a check before the CAS metadata op to ensure that the ensemble does not have an entry id lower than an existing ensemble.
   
   I'm not sure I understand why the client would create a new ensemble with the first entry id being zero. Shouldn't the first entry be the entry id of the last recorded ensemble + 1? Is it zero because because `readLastConfirmedDataComplete` is returning `-1` we are adding 1 to it?




-- 
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.

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