Skip to content

Conversation

@jowiho
Copy link

@jowiho jowiho commented Nov 21, 2022

It's currently not possible to create a ZooKeeperAdmin instance with a custom HostProvider because the ZooKeeperAdmin doesn't expose the necessary ZooKeeper base class constructor. To make this possible, this PR adds ZooKeeperAdmin constructor that passes a given HostProvider parameter to the ZooKeeper base class constructor.

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.

I support this idea.

I have a couple of requests on this patch:

  1. I created this JIRA, can you please update the commit message and the PR title ? https://issues.apache.org/jira/browse/ZOOKEEPER-4656
  2. can you please add a test ?
  3. Add a new overloaded constructor will add some tech debt. Would you please think about adding a Builder API for ZooKeeperAdmin ? this way in the future it will be simpler to add more customisation points

@jowiho jowiho changed the title Allow ZooKeeperAdmin creation with custom HostProvider ZOOKEEPER-4656: Allow ZooKeeperAdmin creation with custom HostProvider Jan 4, 2023
@jowiho jowiho force-pushed the zookeeperadmin-with-custom-hostprovider branch from 4220dfe to 551ea5a Compare January 4, 2023 13:22
@jowiho
Copy link
Author

jowiho commented Jan 4, 2023

Thanks for your feedback @eolivelli, and for creating the JIRA issue. I've added the JIRA ID to the PR title and commit message.

can you please add a test ?

I've considered it, but I couldn't come up with a good way to test it. There currently are no other tests for ZookeeperAdmin. Even if I'd add a new test class, I'd still struggle to add a meaningful test for the constructor overload that I'm adding in this PR. Basically the test would need to verify that the constructor propagates the hostProvider parameter to its ZooKeeper super class. But AFAICT there is no way to access the hostProvider in ZooKeeper directly, in order to test if it's the same as the parameter that I passed to ZookeeperAdmin. Do you have a clever idea on how to test this without an overly complex test setup?

Add a new overloaded constructor will add some tech debt. Would you please think about adding a Builder API for ZooKeeperAdmin ? this way in the future it will be simpler to add more customisation points

I agree that a Builder API would be a very useful addition. But perhaps, in order to keep the PR small, that would better be done in a separate PR?

@eolivelli
Copy link
Contributor

I would prefer to not commit the new constructor and then remove it.
There is no hurry in committing a patch, and sometimes if someone adds a new API it may stay there forever if we forget about it.
So overall I believe that it is better to add the builder and do not add a new public constructor..

For the test, we can a very simple test that bootstraps an instance using the new builder.

@jowiho
Copy link
Author

jowiho commented May 31, 2023

Superseded by #2001

@jowiho jowiho closed this May 31, 2023
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.

2 participants