[GitHub] [bookkeeper] wenbingshen commented on a diff in pull request #3833: [improve] Enhance LedgerFragment toString method

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

wenbingshen commented on code in PR #3833:
URL: https://github.com/apache/bookkeeper/pull/3833#discussion_r1184622195


##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragment.java:
##########
@@ -229,15 +229,16 @@ public void setReplicateType(ReplicateType replicateType) {
     @Override
     public String toString() {
         return String.format("Fragment(LedgerID: %d, FirstEntryID: %d[%d], "
-                + "LastKnownEntryID: %d[%d], Host: %s, Closed: %s)", ledgerId, firstEntryId,
+                + "LastKnownEntryID: %d[%d], Host: %s, Closed: %s, Type: %s)", ledgerId, firstEntryId,
                 getFirstStoredEntryId(), lastKnownEntryId, getLastStoredEntryId(),
-                getAddresses(), isLedgerClosed);
+                getAddresses(), isLedgerClosed, replicateType);
     }
 
     /**
      * ReplicateType.
      */
     public enum ReplicateType {
+        NULL,

Review Comment:
   @hangc0276 @horizonzy Yes, when I first looked at the use of LedgerFragment, I also thought it only had two states, `DATA_LOSS` or `DATA_NOT_ADHERING_PLACEMENT`, when I read the code again, I felt something was different.
   
   `LedgerFragmentReplicator` is used to copy and restore those copies of a ledger segment in a failed bookie. This state initialization is `DATA_LOSS`.
   
   `ReplicationWorker` will first recover the LedgerFragment of `DATA_LOSS` state, and then recover the LedgerFragment of `DATA_NOT_ADHERING_PLACEMENT`.
   
   When `BookKeeperAdmin` is recovering Ledger, LedgerFragment is in `DATA_LOSS` state.
   
   The above LedgerFragment is mainly in recover or replicate, and LedgerFragment has an obvious state.
   
   In addition to the above, there is also a LedgerFragment, which is in an unclear state and mainly occurs inside `LedgerChecker`. `AuditorCheckAllLedgersTask` and `ReplicationWorker` use `LedgerChecker` checkLedger. By default, it will check the first and last Entry of each LedgerFragment of the ledger before the check is completed. , LedgerFragment should not be in `DATA_LOSS` or `DATA_NOT_ADHERING_PLACEMENT`, because after the inspection is completed, the LedgerFragment may be normal, or the replicate may be missing; before and after the inspection is completed, we sometimes print the LedgerFragment through debug, which may cause a wrong state, so I added the `NULL` state is used to identify this situation.
   
   https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java#L476-L492
   ```java
           for (LedgerFragment r : fragments) {
               if (LOG.isDebugEnabled()) {
                   LOG.debug("Checking fragment {}", r);
               }
               try {
                   verifyLedgerFragment(r, allFragmentsCb, percentageOfLedgerFragmentToBeVerified);
               } catch (InvalidFragmentException ife) {
                   LOG.error("Invalid fragment found : {}", r);
                   allFragmentsCb.operationComplete(
                           BKException.Code.IncorrectParameterException, r);
               } catch (BKException e) {
                   LOG.error("BKException when checking fragment : {}", r, e);
               } catch (InterruptedException e) {
                   LOG.error("InterruptedException when checking fragment : {}", r, e);
               }
           }
   ```
   
   `NULL` may be difficult to understand, maybe we can add `INIT` before checking, and `NORMAL` state if the segment not data loss after checking?



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

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