----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27529/#review63960 -----------------------------------------------------------
I'd prefer during refactor not to change behavior (like dropping some branches). it would make the review much easier and make sure the behavior is same as before before we did more improvements. mixing renaming, refactoring with improvement is kind of messy for reviewers. I would go thru the patch again after the comments are addressed, to make sure it doesn't break some corner cases. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerCreateOp.java <https://reviews.apache.org/r/27529/#comment106315> use {} bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java <https://reviews.apache.org/r/27529/#comment106317> why not copyOf and then put(fragmentStartId, ...)? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java <https://reviews.apache.org/r/27529/#comment106318> {} bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java <https://reviews.apache.org/r/27529/#comment106319> {} bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerFragmentReplicator.java <https://reviews.apache.org/r/27529/#comment106320> could you log this as a info message? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106323> doesn't isConflictWith covers #isClosed() and getLastEntryId() checking? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106322> check the result of CAS? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106324> final? rename to bookieFailures? and move it up to be with variables? and wny use List? not Set? the complexity of List is o(n) bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106325> why synchronized on this method? I assume you intend to synchronize on the failure structure. you should reduce synchronization scope, not to introduce potential deadlock in future. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106326> synchronized? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106328> copyOf and add new? or at least make a method to do this in LedgerMetadata. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106330> why you remove 'resolveConflict'? this doesn't seem to be right to me. bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106331> lastAddConfirmed should be in synchronized block. why you move it out? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java <https://reviews.apache.org/r/27529/#comment106333> BKException.Code.OK but failed to update metadataRef, is different from rc != BKException.Code.OK. Could you log different message? bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java <https://reviews.apache.org/r/27529/#comment106337> why you remove version comparison? and it would be better to handle compareAndSet result? at least log a message bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicBookieCheckTest.java <https://reviews.apache.org/r/27529/#comment106341> it'd better to make this as a equivant method. because I saw this builder pattern everywhere in the code. bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConditionalSetTest.java <https://reviews.apache.org/r/27529/#comment106342> this doesn't seem to be aligned with the code? - Sijie Guo On Nov. 13, 2014, 1:56 p.m., Ivan Kelly wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27529/ > ----------------------------------------------------------- > > (Updated Nov. 13, 2014, 1:56 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 > >
