Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[client] new API to check if Bookkeeper client is connected to metadata service #4342

Merged
merged 14 commits into from
Jul 26, 2024

Conversation

congbobo184
Copy link
Contributor

@congbobo184 congbobo184 commented May 6, 2024

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

case Disconnected:
this.metadataServiceAvailable = false;
break;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove line break

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@congbobo184 looks like line break is not removed?

Copy link
Contributor Author

@congbobo184 congbobo184 May 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why remove this? change metadataServiceAvailable twice? @shoothzj

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@congbobo184 line 256 blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, I thought it was to delete break

@eolivelli eolivelli changed the title Bookkeeper client to check metadata service is available [client] Bookkeeper client to check metadata service is available May 6, 2024
@eolivelli eolivelli changed the title [client] Bookkeeper client to check metadata service is available [client] new API to check if Bookkeeper client is connected to metadata service May 6, 2024
Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

in theory we should require a BP for this, but the new API is pretty straightforward and it is not a breaking change

@merlimat @dlg99 @hangc0276

*
* @return the metadata service is available.
*/
CompletableFuture<Boolean> isDriverMetadataServiceAvailable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: add a blank line

@@ -64,6 +66,8 @@
public class ZKMetadataDriverBase implements AutoCloseable {

protected static final String SCHEME = "zk";

protected volatile boolean metadataServiceAvailable;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: add blank line after

@Test
public void testDriverMetadataServiceAvailable()
throws Exception {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits: redundant blank line

Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I find some nits.

*
* @return the metadata service is available.
*/
CompletableFuture<Boolean> isDriverMetadataServiceAvailable();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a default implementation to avoid breaking the interface

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is not a required thing for the bookkeeper. I think it should be ok to implement a default handle. @shoothzj

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we only need one blank space here.

@congbobo184
Copy link
Contributor Author

rerun failure checks

Copy link
Member

@shoothzj shoothzj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default impl needs discuss, I have started a mail thread: https://lists.apache.org/thread/fk02bz4lggz086kgwwxdw2yv1ktn7x35

cc @eolivelli @zymap

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as discussed in the mailing list, having a default implementation is error prone
it is better to not have a default implementation

@congbobo184
Copy link
Contributor Author

@shoothzj @eolivelli please review again, thanks

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Signed-off-by: ZhangJian He <[email protected]>
@shoothzj shoothzj merged commit d92bd20 into apache:master Jul 26, 2024
23 checks passed
@shoothzj
Copy link
Member

Thanks for your contribution. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants