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

Cleanup registration client interface #1116

Closed

Conversation

sijie
Copy link
Member

@sijie sijie commented Feb 2, 2018

Descriptions of the changes in this PR:

This change is mainly to remove zookeeper reference from metadata interface. The existence of Optional<ZooKeeper> is to allow passing an external zookeeper client to bookkeeper, so bookkeeper can reuse that client instance. That is useful for services like pulsar broker, which they can only instantiate a zookeeper client and pass the zookeeper client around to construct bookkeeper client. However, this pollutes the registration client interface, change the method to Optional<Object> as an optional context object and let the implementation interpret it.

@sijie
Copy link
Member Author

sijie commented Feb 2, 2018

@jiazhai since you did the interface abstraction, can you take a look to see if this work for you.

@merlimat since pulsar is the main user of external zookeeper instance, can you review to make sure it is okay from your perspective.

to be clear on this change, it would be ideal to remove that field completely. however since bookkeeper still supports passing external zookeeper client and most likely it won't be gone for any time soon, so I stick to just change it to Optional<Object> to remove zookeeper reference from metadata interfaces entirely. Let the implementation itself interpret this context object.

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

Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

+1. Thanks for this.

@sijie sijie closed this in 1286970 Feb 3, 2018
@sijie sijie deleted the clean_registration_client_interface branch July 16, 2018 02:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants