You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@bookkeeper.apache.org by Ivan Kelly <iv...@apache.org> on 2014/11/03 18:02:36 UTC
Review Request 27529: BOOKKEEPER-795 Race condition causes writes to
hang if ledger is fences
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27529/
-----------------------------------------------------------
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
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by Ivan Kelly <iv...@apache.org>.
> 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
>
>
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by fp...@apache.org.
> On Nov. 12, 2014, 4:33 p.m., fpj wrote:
> > I quite like the changes here, they make sense to me. I just have one clarification question and a small suggestion below.
I'm happy with the changes. Any other comment here or should I get this in?
> 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?
>
> Ivan Kelly wrote:
> #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.
Renaming sounds like a good idea indeed to avoid confusion.
- fpj
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27529/#review60997
-----------------------------------------------------------
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
>
>
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by fp...@apache.org.
-----------------------------------------------------------
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
>
>
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27529/#review61600
-----------------------------------------------------------
I will review this over this weekend.
- 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
>
>
Re: Would you please confirm the understanding of maxOutstandingRequests
in GarbageCollector?
Posted by Ivan Kelly <iv...@apache.org>.
I'll reply on the JIRA.
On Tue, Dec 16, 2014 at 6:51 AM, Jia Zhai <zh...@gmail.com> wrote:
> Hi Ivan,
>
> Thanks a ton for your explain. It is very clear.
>
> If you have time, Would you please help take a look at bookkeeper-827?
> https://issues.apache.org/jira/browse/BOOKKEEPER-827
>
> After this enhancement, user could choose to use "compactionRate"
> and "maxOutstandingRequests" either "by entries" or "by bytes". And new
> parameter "maxOutstandingRequestsBytes" stands for number of bytes that
> entries have occupied before flush.
>
>
> Thanks a lot.
>
> -Jia
>
> On Tue, Dec 16, 2014 at 12:14 AM, Ivan Kelly <iv...@apache.org> wrote:
> >
> > No, in this case it is talking about the offsets of the entries.
> >
> > When we do GC, we read from the old (partially empty) log and copy any
> > non-deleted entries we find into the new entry log. This means that the
> > index location for that entry needs to change to point to the new
> entrylog
> > and new offset. We cannot update the index though, until the new entry
> log
> > has flushed. Otherwise, we could update the index, then crash, the new
> > index entry would point to memory which had never been flushed. We cannot
> > flush the entry log for each entry, because that would slow everything
> > down. So we batch. We copy over 'maxOutstandingRequests' entries, keep a
> > record of the new offsets, and then wait for a flush of the entrylog, or
> > trigger one (I can't remember which). Once the flush has occurred, the
> > offsets can be added to the index.
> >
> > Let us know if this isn't clear, as it most likely isn't :)
> >
> > -Ivan
> >
> > On Sat, Dec 13, 2014 at 4:12 PM, zhaijia03@gmail.com <
> zhaijia03@gmail.com>
> > wrote:
> >
> > > Hi,
> > >
> > > I am preparing a patch for
> > > https://issues.apache.org/jira/browse/BOOKKEEPER-827 , in which try to
> > > turn maxOutstandingRequests from "by entries" to "by bytes".
> > > while reading the part of comments at the front of
> > > setCompactionMaxOutstandingRequests(), it makes me a little confusing:
> > > "A higher value for this parameter means more memory will be used for
> > > offsets"
> > > <=== here, I thought the memory should mainly be occupied by the
> entries
> > > that added into "FileChannel", but not flushed. right?
> > >
> > >
> >
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
> > > : line 1231
> > > ------------------------
> > > /**
> > > * Set the maximum number of entries which can be compacted without
> > > flushing.
> > > *
> > > * When compacting, the entries are written to the entrylog and the
> > > new offsets
> > > * are cached in memory. Once the entrylog is flushed the index is
> > > updated with
> > > * the new offsets. This parameter controls the number of entries
> > > added to the
> > > * entrylog before a flush is forced. A higher value for this
> > > parameter means
> > > * more memory will be used for offsets. Each offset consists of 3
> > > longs.
> > > *
> > > * This parameter should _not_ be modified unless you know what
> > you're
> > > doing.
> > > * The default is 100,000.
> > > *
> > > * @param maxOutstandingRequests number of entries to compact
> before
> > > flushing
> > > *
> > > * @return ServerConfiguration
> > > */
> > > public ServerConfiguration setCompactionMaxOutstandingRequests(int
> > > maxOutstandingRequests) {
> > > setProperty(COMPACTION_MAX_OUTSTANDING_REQUESTS,
> > > maxOutstandingRequests);
> > > return this;
> > > }
> > > ------------------------
> > >
> > > Thanks a lot.
> > > -Jia
> > >
> >
>
Re: Would you please confirm the understanding of maxOutstandingRequests
in GarbageCollector?
Posted by Jia Zhai <zh...@gmail.com>.
Hi Ivan,
Thanks a ton for your explain. It is very clear.
If you have time, Would you please help take a look at bookkeeper-827?
https://issues.apache.org/jira/browse/BOOKKEEPER-827
After this enhancement, user could choose to use "compactionRate"
and "maxOutstandingRequests" either "by entries" or "by bytes". And new
parameter "maxOutstandingRequestsBytes" stands for number of bytes that
entries have occupied before flush.
Thanks a lot.
-Jia
On Tue, Dec 16, 2014 at 12:14 AM, Ivan Kelly <iv...@apache.org> wrote:
>
> No, in this case it is talking about the offsets of the entries.
>
> When we do GC, we read from the old (partially empty) log and copy any
> non-deleted entries we find into the new entry log. This means that the
> index location for that entry needs to change to point to the new entrylog
> and new offset. We cannot update the index though, until the new entry log
> has flushed. Otherwise, we could update the index, then crash, the new
> index entry would point to memory which had never been flushed. We cannot
> flush the entry log for each entry, because that would slow everything
> down. So we batch. We copy over 'maxOutstandingRequests' entries, keep a
> record of the new offsets, and then wait for a flush of the entrylog, or
> trigger one (I can't remember which). Once the flush has occurred, the
> offsets can be added to the index.
>
> Let us know if this isn't clear, as it most likely isn't :)
>
> -Ivan
>
> On Sat, Dec 13, 2014 at 4:12 PM, zhaijia03@gmail.com <zh...@gmail.com>
> wrote:
>
> > Hi,
> >
> > I am preparing a patch for
> > https://issues.apache.org/jira/browse/BOOKKEEPER-827 , in which try to
> > turn maxOutstandingRequests from "by entries" to "by bytes".
> > while reading the part of comments at the front of
> > setCompactionMaxOutstandingRequests(), it makes me a little confusing:
> > "A higher value for this parameter means more memory will be used for
> > offsets"
> > <=== here, I thought the memory should mainly be occupied by the entries
> > that added into "FileChannel", but not flushed. right?
> >
> >
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
> > : line 1231
> > ------------------------
> > /**
> > * Set the maximum number of entries which can be compacted without
> > flushing.
> > *
> > * When compacting, the entries are written to the entrylog and the
> > new offsets
> > * are cached in memory. Once the entrylog is flushed the index is
> > updated with
> > * the new offsets. This parameter controls the number of entries
> > added to the
> > * entrylog before a flush is forced. A higher value for this
> > parameter means
> > * more memory will be used for offsets. Each offset consists of 3
> > longs.
> > *
> > * This parameter should _not_ be modified unless you know what
> you're
> > doing.
> > * The default is 100,000.
> > *
> > * @param maxOutstandingRequests number of entries to compact before
> > flushing
> > *
> > * @return ServerConfiguration
> > */
> > public ServerConfiguration setCompactionMaxOutstandingRequests(int
> > maxOutstandingRequests) {
> > setProperty(COMPACTION_MAX_OUTSTANDING_REQUESTS,
> > maxOutstandingRequests);
> > return this;
> > }
> > ------------------------
> >
> > Thanks a lot.
> > -Jia
> >
>
Re: Would you please confirm the understanding of maxOutstandingRequests
in GarbageCollector?
Posted by Ivan Kelly <iv...@apache.org>.
No, in this case it is talking about the offsets of the entries.
When we do GC, we read from the old (partially empty) log and copy any
non-deleted entries we find into the new entry log. This means that the
index location for that entry needs to change to point to the new entrylog
and new offset. We cannot update the index though, until the new entry log
has flushed. Otherwise, we could update the index, then crash, the new
index entry would point to memory which had never been flushed. We cannot
flush the entry log for each entry, because that would slow everything
down. So we batch. We copy over 'maxOutstandingRequests' entries, keep a
record of the new offsets, and then wait for a flush of the entrylog, or
trigger one (I can't remember which). Once the flush has occurred, the
offsets can be added to the index.
Let us know if this isn't clear, as it most likely isn't :)
-Ivan
On Sat, Dec 13, 2014 at 4:12 PM, zhaijia03@gmail.com <zh...@gmail.com>
wrote:
> Hi,
>
> I am preparing a patch for
> https://issues.apache.org/jira/browse/BOOKKEEPER-827 , in which try to
> turn maxOutstandingRequests from "by entries" to "by bytes".
> while reading the part of comments at the front of
> setCompactionMaxOutstandingRequests(), it makes me a little confusing:
> "A higher value for this parameter means more memory will be used for
> offsets"
> <=== here, I thought the memory should mainly be occupied by the entries
> that added into "FileChannel", but not flushed. right?
>
> bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
> : line 1231
> ------------------------
> /**
> * Set the maximum number of entries which can be compacted without
> flushing.
> *
> * When compacting, the entries are written to the entrylog and the
> new offsets
> * are cached in memory. Once the entrylog is flushed the index is
> updated with
> * the new offsets. This parameter controls the number of entries
> added to the
> * entrylog before a flush is forced. A higher value for this
> parameter means
> * more memory will be used for offsets. Each offset consists of 3
> longs.
> *
> * This parameter should _not_ be modified unless you know what you're
> doing.
> * The default is 100,000.
> *
> * @param maxOutstandingRequests number of entries to compact before
> flushing
> *
> * @return ServerConfiguration
> */
> public ServerConfiguration setCompactionMaxOutstandingRequests(int
> maxOutstandingRequests) {
> setProperty(COMPACTION_MAX_OUTSTANDING_REQUESTS,
> maxOutstandingRequests);
> return this;
> }
> ------------------------
>
> Thanks a lot.
> -Jia
>
Would you please confirm the understanding of maxOutstandingRequests in GarbageCollector?
Posted by "zhaijia03@gmail.com" <zh...@gmail.com>.
Hi,
I am preparing a patch for https://issues.apache.org/jira/browse/BOOKKEEPER-827 , in which try to turn maxOutstandingRequests from "by entries" to "by bytes".
while reading the part of comments at the front of setCompactionMaxOutstandingRequests(), it makes me a little confusing:
"A higher value for this parameter means more memory will be used for offsets"
<=== here, I thought the memory should mainly be occupied by the entries that added into "FileChannel", but not flushed. right?
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java : line 1231
------------------------
/**
* Set the maximum number of entries which can be compacted without flushing.
*
* When compacting, the entries are written to the entrylog and the new offsets
* are cached in memory. Once the entrylog is flushed the index is updated with
* the new offsets. This parameter controls the number of entries added to the
* entrylog before a flush is forced. A higher value for this parameter means
* more memory will be used for offsets. Each offset consists of 3 longs.
*
* This parameter should _not_ be modified unless you know what you're doing.
* The default is 100,000.
*
* @param maxOutstandingRequests number of entries to compact before flushing
*
* @return ServerConfiguration
*/
public ServerConfiguration setCompactionMaxOutstandingRequests(int maxOutstandingRequests) {
setProperty(COMPACTION_MAX_OUTSTANDING_REQUESTS, maxOutstandingRequests);
return this;
}
------------------------
Thanks a lot.
-Jia
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by Ivan Kelly <iv...@apache.org>.
> 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
>
>
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by Sijie Guo <gu...@gmail.com>.
-----------------------------------------------------------
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
>
>
Re: Review Request 27529: BOOKKEEPER-795 Race condition causes writes
to hang if ledger is fences
Posted by Ivan Kelly <iv...@apache.org>.
-----------------------------------------------------------
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.
Changes
-------
new patch addresses flavio's comments, and fixes a findbugs issue.
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 (updated)
-----
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