[GitHub] [zookeeper] jowiho commented on pull request #1947: ZOOKEEPER-4656: Allow ZooKeeperAdmin creation with custom HostProvider

[GitBox <gi...@apache.org>]

jowiho commented on PR #1947:
URL: https://github.com/apache/zookeeper/pull/1947#issuecomment-1370934346

   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?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@zookeeper.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org