----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27529/#review60997 -----------------------------------------------------------
I quite like the changes here, they make sense to me. I just have one clarification question and a small suggestion below. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment102459> I'm a bit confused by the synchronization here. The implementation of handleBookieFailure with a different signature is synchronized but this one isn't. It sounds like handleBookieFailure(BookieFailureEvent failure) is invoked outside handleBookieFailure(final BookieSocketAddress addr, final int bookieIndex) and it isn't clear to me if the synchroization as proposed is right. Have you actually checked? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment102460> Small suggestion: I'd rather call it tmpMetadata or something similar rather than numbering variables like this. - fpj On Nov. 3, 2014, 5:02 p.m., Ivan Kelly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27529/ > ----------------------------------------------------------- > > (Updated Nov. 3, 2014, 5:02 p.m.) > > > Review request for bookkeeper. > > > Bugs: BOOKKEEPER-795 > https://issues.apache.org/jira/browse/BOOKKEEPER-795 > > > Repository: bookkeeper-git > > > Description > ------- > > Made ledger metadata immutable > > Ensembles are now ImmutableMap<Long, ImmutableList<BookieSocketAddress>>. > The LedgerManager manager interface has changed to avoid implicitly > updating the version of a LedgerMetadata object. Recovery, Closing and > handling bookie failures have now changed, as there are no longer merge > operations. If a write to zookeeper fails, then > recovery/closing/failureHandling are retried with the new metadata, > provided that assumptions are not broken. > > The interesting changes are in LedgerHandle and LedgerMetadata. > > There are a lot of changes from ArrayList -> ImmutableList,List > Also a lot of changes from currentEnsemble -> getCurrentEnsemble. > > One test had to change. With the old code, if someone fenced a ledger, > closing would fail. This isn't always the case now. If there's lac of > the writing ledger matches the lastEntryId of the new metadata, then > close can succeed. ConditionalSetTest has changed to reflect this. > > > Diffs > ----- > > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/BookKeeperAdmin.java > 18a801c > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerChecker.java > 3f2580f > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java > fe223af > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragment.java > 6aadb8a > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java > 4501524 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java > 7204d6c > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerMetadata.java > a20f34a > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerRecoveryOp.java > 7ed7aa2 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingAddOp.java > 1b92d09 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/PendingReadOp.java > e548f3d > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadLastConfirmedOp.java > af21f44 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java > 8de4092 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/TryReadLastConfirmedOp.java > 01b81c9 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/AbstractZkLedgerManager.java > 0fc8afe > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/CleanupLedgerManager.java > a873112 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/FlatLedgerManager.java > 2bc4258 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/HierarchicalLedgerManager.java > 7f2df73 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManager.java > 7229028 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/MSLedgerManagerFactory.java > 2510b89 > > bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/BookieLedgerIndexer.java > 1b4efaf > > bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/ReplicationWorker.java > 02154e5 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/CompactionTest.java > ef4cea8 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieRecoveryTest.java > f18e159 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/BookieWriteLedgerTest.java > 7b77c48 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/ClientUtil.java > dc43b2c > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerCloseTest.java > eef56b0 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerHandleAdapter.java > 9af527a > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/LedgerRecoveryTest.java > f54cde1 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/SlowBookieTest.java > 521d1e3 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerChecker.java > eb61c21 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestLedgerFragmentReplication.java > e4f744f > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestSpeculativeRead.java > 2a6c71d > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestTryReadLastConfirmed.java > ac0afb6 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/client/TestWatchEnsembleChange.java > eb833a3 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/meta/GcLedgersTest.java > 19aab44 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicBookieCheckTest.java > 91aae77 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/BookieAutoRecoveryTest.java > 72fd11c > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestAutoRecoveryAlongWithBookieServers.java > e1ccf68 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/TestReplicationWorker.java > d47e4b1 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/BookKeeperClusterTestCase.java > 9662777 > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConditionalSetTest.java > a06accd > > Diff: https://reviews.apache.org/r/27529/diff/ > > > Testing > ------- > > All tests pass > > > Thanks, > > Ivan Kelly > >
