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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
