> On Nov. 12, 2014, 4:33 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 888 > > <https://reviews.apache.org/r/27529/diff/1/?file=747433#file747433line888> > > > > 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?
#handleBookieFailure(BookieFailureEvent) doesn't actually mutate any shared state, so it doesn't need to be synchronized. #handleBookieFailure(BookieSocketAddress, int) modifies the failures set, which is the failures that are currently being handled. I could use a concurrentset here, but this is there's no blocking operations in handleBookieFailure in any case, and bookieFailureHandled calls unsetSuccessAndSendWriteRequest, which needs to be synchronized, so I'd prefer to leave it as is. I should rename #handleBookieFailure(BookieFailureEvent) though to avoid confusion. > On Nov. 12, 2014, 4:33 p.m., fpj wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 982 > > <https://reviews.apache.org/r/27529/diff/1/?file=747433#file747433line982> > > > > Small suggestion: I'd rather call it tmpMetadata or something similar > > rather than numbering variables like this. Will rename. I actually plan to get rid of this code later by extracting version from the LedgerMetadata object completely, but this change was big enough as it was. - Ivan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27529/#review60997 ----------------------------------------------------------- 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 > >
