ZOOKEEPER-4697: Add Builder to construct ZooKeeper and ZooKeeperAdmin #2001
Conversation
Currently, there are 10 constructor variants for `ZooKeeper` and 4 for `ZooKeeperAdmin`. It is enough for us to resort to a builder. The `build` method throws `IOException` to make it a drop-in replacement of existing constructors of `ZooKeeper`. This pr also unify body of `ZooKeeper` constructor to one. Previously, there are diverged to two. One has `sessionId` and `sessionPasswd`, and another doesn't have. This pr uses `sessionId == 0` to differentiate the two as it is used in server side to differentiate session create and reconnect.
![sonatype-lift[bot]](https://avatars.githubusercontent.com/in/9843?s=60&v=4){kind=small}
|
@eolivelli @jowiho Can you please take a look at this ? I think it supersedes ZOOKEEPER-4656(#1947) though I did not realize this in creating ZOOKEEPER-4697. |
|
I like this proposal. Will check it in this week. |
|
@kezhuw, I agree that your solution supersedes ZOOKEEPER-4656(#1947). I never got round to writing a builder for ZookeeperAdmin, mainly because the complexity of testing it. So I'm happy to see that you did just that. Thanks! I've closed my PR (#1947) in favor of this PR. |
| * @throws IOException from constructor of ZooKeeper instance or wrapper of no IO exception | ||
| */ | ||
| @SuppressWarnings("unchecked") | ||
| public <T extends ZooKeeper> T build(Class<T> clazz) throws IOException { |
There was a problem hiding this comment.
Please don't go this way.
This is totally out of our control.
If developers know the class they can instantiate it explicitly.
It it fine to add a buildZooKeeperAdmin() method
There was a problem hiding this comment.
I felt similar(multiple ways to construct). I think there are candidates for us to go:
- Only
buildandbuildAdminforZooKeeperandZooKeeperAdmin. For other derivations, they should resort toZooKeeperBuilder.toOptionsandCustomZooKeeper(ZooKeeperOptions)for full options customization. - Restrict
ZooKeeperBuilder.toOptionsto some level of private, soCustomZooKeeper(ZooKeeperOptions)is a simple hook forZooKeeperBuilder. This way clients are encouraged(or forced) to useZooKeeperBuilder::build(Class<T> class)to constructCustomZooKeeper. - Combine above two and support only
ZooKeeperandZooKeeperAdmin. No third party derivations are supported or encouraged.
Maybe we can go 3 and unleash restriction in future if requested ? Currently, all other derivations are test purpose.
For "some level of private", I means @InterfaceAudience.Private or moving to package where ZooKeeper resides in.
There was a problem hiding this comment.
I have added a follow up commit to build only ZooKeeper and ZooKeeperAdmin(e.g. candidate 3 from above).
I limited ZooKeeper(ZooKeeperOptions options), ZooKeeperAdmin(ZooKeeperOptions options) and ZooKeeperOptions to private using InterfaceAudience.Private now. ZooKeeperAdmin resides in different package than ZooKeeper, so I have resorted to InterfaceAudience.Private somewhere.
There was a problem hiding this comment.
No third party derivations are supported or encouraged
Anyone inherits ZooKeeper should be fine to build their own constructing method. And yes, we don't encourage that. If it's a common use case, it can go to upstream; if it's not, the upstream can be refactored to accept combination over inheritance. ZooKeeper is one of the key abstractions we deliver to users.
kezhuw
changed the title
tisonkun
left a comment
tisonkun
left a comment
There was a problem hiding this comment.
The currrent implementation looks good.
Thank you!
|
cc @eolivelli may you give another look on this PR? |
|
@tisonkun @eolivelli Shall we merge this ? |
…apache#2001) * ZOOKEEPER-4697: Add Builder to construct ZooKeeper and its derivations Currently, there are 10 constructor variants for `ZooKeeper` and 4 for `ZooKeeperAdmin`. It is enough for us to resort to a builder. The `build` method throws `IOException` to make it a drop-in replacement of existing constructors of `ZooKeeper`. This pr also unify body of `ZooKeeper` constructor to one. Previously, there are diverged to two. One has `sessionId` and `sessionPasswd`, and another doesn't have. This pr uses `sessionId == 0` to differentiate the two as it is used in server side to differentiate session create and reconnect. * Restrict Builder to only ZooKeeper and ZooKeeperAdmin
…apache#2001) * ZOOKEEPER-4697: Add Builder to construct ZooKeeper and its derivations Currently, there are 10 constructor variants for `ZooKeeper` and 4 for `ZooKeeperAdmin`. It is enough for us to resort to a builder. The `build` method throws `IOException` to make it a drop-in replacement of existing constructors of `ZooKeeper`. This pr also unify body of `ZooKeeper` constructor to one. Previously, there are diverged to two. One has `sessionId` and `sessionPasswd`, and another doesn't have. This pr uses `sessionId == 0` to differentiate the two as it is used in server side to differentiate session create and reconnect. * Restrict Builder to only ZooKeeper and ZooKeeperAdmin
4 participants
Currently, there are 10 constructor variants for
ZooKeeperand 4 forZooKeeperAdmin. It is enough for us to resort to a builder.The
buildmethod throwsIOExceptionto make it a drop-in replacement of existing constructors ofZooKeeper.This pr also unify body of
ZooKeeperconstructor to one. Previously, there are diverged to two. One hassessionIdandsessionPasswd, and another doesn't have. This pr usessessionId == 0to differentiate the two as it is used in server side to differentiate session create and reconnect.