[client] new API to check if Bookkeeper client is connected to metadata service #4342
Conversation
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
Outdated
Show resolved
Hide resolved
| case Disconnected: | ||
| this.metadataServiceAvailable = false; | ||
| break; | ||
|
|
There was a problem hiding this comment.
@congbobo184 looks like line break is not removed?
There was a problem hiding this comment.
why remove this? change metadataServiceAvailable twice? @shoothzj
There was a problem hiding this comment.
@congbobo184 line 256 blank line?
There was a problem hiding this comment.
sorry, I thought it was to delete break
bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/zk/ZKMetadataDriverBase.java
Outdated
Show resolved
Hide resolved
eolivelli
changed the title
eolivelli
changed the title
| * | ||
| * @return the metadata service is available. | ||
| */ | ||
| CompletableFuture<Boolean> isDriverMetadataServiceAvailable(); |
| @@ -64,6 +66,8 @@ | |||
| public class ZKMetadataDriverBase implements AutoCloseable { | |||
|
|
|||
| protected static final String SCHEME = "zk"; | |||
|
|
|||
| protected volatile boolean metadataServiceAvailable; | |||
| @Test | ||
| public void testDriverMetadataServiceAvailable() | ||
| throws Exception { | ||
|
|
| * | ||
| * @return the metadata service is available. | ||
| */ | ||
| CompletableFuture<Boolean> isDriverMetadataServiceAvailable(); |
There was a problem hiding this comment.
Add a default implementation to avoid breaking the interface
There was a problem hiding this comment.
I doubt that add default implementation is a good idea. First, we don't guarantee/make ABI comptabile between minor releases; Second, People whole implement metadata driver should implement this.
cc @eolivelli @dlg99
There was a problem hiding this comment.
Since it is not a required thing for the bookkeeper. I think it should be ok to implement a default handle. @shoothzj
There was a problem hiding this comment.
@zymap This default interface might be error prone. I have started a mail thread. I think we can continue discuss in https://lists.apache.org/thread/fk02bz4lggz086kgwwxdw2yv1ktn7x35
| * Bookkeeper Client API driver metadata service available test. | ||
| */ | ||
| public class DriverMetadataServiceAvailableTest extends BookKeeperClusterTestCase { | ||
|
|
There was a problem hiding this comment.
nit: we only need one blank space here.
|
rerun failure checks |
There was a problem hiding this comment.
I think the default impl needs discuss, I have started a mail thread: https://lists.apache.org/thread/fk02bz4lggz086kgwwxdw2yv1ktn7x35
cc @eolivelli @zymap
|
@shoothzj @eolivelli please review again, thanks |
Signed-off-by: ZhangJian He <[email protected]>
|
Thanks for your contribution. :) |
Motivation
when zookeeper disconnect, use bookkeeper api to create new ledger will fail. add a new api to check metadata driver is available, if not available, use current ledger continue and don't rollover ledger
** tip **
Although it may not solve all issues since it's not atomic, it will reduce the losses caused by zk unavailability.
Modification
add a new api named isDriverMetadataServiceAvailable