> On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 344 > > <https://reviews.apache.org/r/27529/diff/2/?file=762242#file762242line344> > > > > doesn't isConflictWith covers #isClosed() and getLastEntryId() checking?
LedgerHandle#lastAddConfirmed isn't LedgerMetadata#lastEntryId. I want to check against what the ledger says, not what the metadata says. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 347 > > <https://reviews.apache.org/r/27529/diff/2/?file=762242#file762242line347> > > > > check the result of CAS? Doesn't need to, going to retry anyhow. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 879 > > <https://reviews.apache.org/r/27529/diff/2/?file=762242#file762242line879> > > > > synchronized? call to unsetSuccess... must be synchronized anyhow. so pretty much everything in the method must be. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 1019 > > <https://reviews.apache.org/r/27529/diff/2/?file=762242#file762242line1019> > > > > why you remove 'resolveConflict'? this doesn't seem to be right to me. Because there's no longer a conflict to resolve. Since we haven't modified the metadata in use, and we have just read the current metadata from zk, we can just either retry or fail (depending on the state) > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/LedgerHandle.java, > > line 1134 > > <https://reviews.apache.org/r/27529/diff/2/?file=762242#file762242line1134> > > > > lastAddConfirmed should be in synchronized block. why you move it out? doesn't need to be in this case, since there's nothing concurrent running with #recover() is called. It was causing findbug issues when it was in the sync block. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/main/java/org/apache/bookkeeper/client/ReadOnlyLedgerHandle.java, > > line 59 > > <https://reviews.apache.org/r/27529/diff/2/?file=762248#file762248line59> > > > > why you remove version comparison? > > > > and it would be better to handle compareAndSet result? at least log a > > message Since we only ever update with what has been read from zookeeper, and zookeeper guarantee the order that we see these updates, and as the occurred is checked in #onChanged, it's not needed here. Adding a log. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/replication/AuditorPeriodicBookieCheckTest.java, > > line 116 > > <https://reviews.apache.org/r/27529/diff/2/?file=762272#file762272line116> > > > > it'd better to make this as a equivant method. because I saw this > > builder pattern everywhere in the code. I did think of that when doing the patch originally. However, each case is slightly different, and so much so that I couldn't see a clear way to do it. > On Dec. 5, 2014, 5:51 a.m., Sijie Guo wrote: > > bookkeeper-server/src/test/java/org/apache/bookkeeper/test/ConditionalSetTest.java, > > line 106 > > <https://reviews.apache.org/r/27529/diff/2/?file=762277#file762277line106> > > > > this doesn't seem to be aligned with the code? clarified the comment. - Ivan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27529/#review63960 ----------------------------------------------------------- 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 > >
