horizonzy commented on code in PR #3917:
URL: https://github.com/apache/bookkeeper/pull/3917#discussion_r1168134657
##########
bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java:
##########
@@ -1158,6 +1158,33 @@ public void
testRWShutDownInTheCaseOfZKOperationFailures() throws Exception {
}
}
+ @Test
+ public void testReplicateEmptyOpenStateLedger() throws Exception {
+ LedgerHandle lh = bkc.createLedger(3, 3, 2,
BookKeeper.DigestType.CRC32, TESTPASSWD);
+ assertFalse(lh.getLedgerMetadata().isClosed());
+
+ List<BookieId> firstEnsemble =
lh.getLedgerMetadata().getAllEnsembles().firstEntry().getValue();
Review Comment:
`firstEnsemble` is unused
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java:
##########
@@ -561,6 +563,14 @@ private boolean rereplicate(long ledgerIdToReplicate)
throws InterruptedExceptio
*
* <p>Missing bookies in closed ledgers are fine, as we know the last
confirmed add, so
* we can tell which entries are supposed to exist and rereplicate them if
necessary.
+ *
+ * <p>Another corner case is that there are multiple ensembles in the
ledger and the last
+ * segment/ensemble is open, but nothing has been written to some quorums
in the ensemble.
+ * For the v2 protocol, this ledger's lastAddConfirm entry is the last
segment/ensemble's `key - 2`,
+ * not `key - 2`, the explanation please refer to:
https://github.com/apache/bookkeeper/pull/3917.
+ * If we treat the penultimate segment/ensemble as closed state, we will
can't replicate
+ * the last entry in the segment. So in this case, we should also check if
the penultimate
+ * segment/ensemble has missing bookies.
*/
private boolean isLastSegmentOpenAndMissingBookies(LedgerHandle lh) throws
BKException {
LedgerMetadata md = admin.getLedgerMetadata(lh);
Review Comment:
`md` is unused.
##########
bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java:
##########
@@ -570,6 +580,10 @@ private boolean
isLastSegmentOpenAndMissingBookies(LedgerHandle lh) throws BKExc
SortedMap<Long, ? extends List<BookieId>> ensembles =
admin.getLedgerMetadata(lh).getAllEnsembles();
List<BookieId> finalEnsemble = ensembles.get(ensembles.lastKey());
+ if (ensembles.size() > 1 && lh.getLastAddConfirmed() <
ensembles.lastKey() - 1) {
+ finalEnsemble = new ArrayList<>(finalEnsemble);
Review Comment:
`finalEnsemble = new ArrayList<>(finalEnsemble);` may be meaningless.
--
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]
