[GitHub] [bookkeeper] eolivelli commented on a diff in pull request #3239: ISSUE 3220: Autorecovery does not process underreplicated empty ledgers

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

eolivelli commented on code in PR #3239:
URL: https://github.com/apache/bookkeeper/pull/3239#discussion_r855802565


##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java:
##########
@@ -383,6 +383,7 @@ public void testEmptyLedgerLosesQuorumEventually() throws Exception {
         LOG.info("Killing last bookie, {}, in ensemble {}", replicaToKill,
                  lh.getLedgerMetadata().getAllEnsembles().get(0L));
         killBookie(replicaToKill);
+        startNewBookie();

Review Comment:
   why are we changing this existing test ?



##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java:
##########
@@ -88,7 +98,7 @@ public void testDecommissionBookie() throws Exception {
          */
         bkAdmin.decommissionBookie(BookieImpl.getBookieId(killedBookieConf));
         bkAdmin.triggerAudit();
-        Thread.sleep(500);
+        Thread.sleep(5000);

Review Comment:
   out of the scope of this PR, but in the future it would be better to wait for a specific condition, in order to reduce flakyness



##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java:
##########
@@ -130,11 +144,16 @@ public void testDecommissionForLedgersWithMultipleSegmentsAndNotWriteClosed() th
             lh4.addEntry(j, "data".getBytes());
         }
 
+        // avoiding autorecovery fencing the ledger
+        servers.forEach(srv -> srv.stopAutoRecovery());

Review Comment:
   I am not sure that "stopAutoRecovery" waits for autoRecovery to be totally stopped, maybe there is still some task running?



##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieDecommissionTest.java:
##########
@@ -44,19 +49,23 @@ public class BookieDecommissionTest extends BookKeeperClusterTestCase {
 
     public BookieDecommissionTest() {
         super(NUM_OF_BOOKIES, 480);
-        baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(30000));
+        baseConf.setOpenLedgerRereplicationGracePeriod(String.valueOf(1000));
         setAutoRecoveryEnabled(true);
     }
 
     @FlakyTest("https://github.com/apache/bookkeeper/issues/502")
+    @Test

Review Comment:
   so basically we were not running this test anymore.
   
   great to see this running again



##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java:
##########
@@ -238,7 +237,13 @@ private void verifyLedgerFragment(LedgerFragment fragment,
             if (lastStored != LedgerHandle.INVALID_ENTRY_ID) {
                 throw new InvalidFragmentException();

Review Comment:
   what about taking this chance of changing the code to adding a meaningful message to all the InvalidFragmentException thrown in this method ?



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