You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2022/11/21 11:21:35 UTC

[GitHub] [zookeeper] jowiho opened a new pull request, #1947: Allow ZooKeeperAdmin creation with custom HostProvider

jowiho opened a new pull request, #1947:
URL: https://github.com/apache/zookeeper/pull/1947

   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.


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

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

Posted by "jowiho (via GitHub)" <gi...@apache.org>.
jowiho closed pull request #1947: ZOOKEEPER-4656: Allow ZooKeeperAdmin creation with custom HostProvider
URL: https://github.com/apache/zookeeper/pull/1947


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

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

Posted by "jowiho (via GitHub)" <gi...@apache.org>.
jowiho commented on PR #1947:
URL: https://github.com/apache/zookeeper/pull/1947#issuecomment-1570089463

   Superseded by #2001


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

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

Posted by 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

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

Posted by GitBox <gi...@apache.org>.
eolivelli commented on PR #1947:
URL: https://github.com/apache/zookeeper/pull/1947#issuecomment-1370997242

   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.
   
   
   


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