CURATOR-704. Add server compatibility check support #497
Conversation
Add new interface ZookeeperCompatibility to represent server compatibility in addition to the existing Compatibility class (which represents client compatibility). Enhance CuratorFramework to accept ZookeeperCompatibility instance, allowing user to specify which server version to target (default is LATEST).
| */ | ||
| public interface ZookeeperCompatibility { | ||
| public enum Version implements ZookeeperCompatibility { | ||
| VERSION_3_5(false), |
There was a problem hiding this comment.
I am not sure that enumerating the ZK versions will pay back in the future.
What about letting the user build this object?
And we provide a default instance with all the known features
There was a problem hiding this comment.
I'm okay removing the enum. Do you think the user should just create the concrete class or would you prefer to introduce a Builder class to help with it?
There was a problem hiding this comment.
I guess we probably are pursuing something similar to ClientBuilder::assume_server_version(I am the author). My thought was that client library know server compatibility(capabilities, bugs and etc.) given a server version.
So, I think it is Curator's responsibility to build the compatibility matrix and do the dirty work but not the caller. I think we probably need only a server version (the class and the LATEST) in api side.
There was a problem hiding this comment.
So I changed the code to remove the enum and replace it with a class + builder combo. Let me know if you're okay with this pattern
There was a problem hiding this comment.
I think all we want could be a simple (major, minor, patch) tuple. Curator can derive the compatibility matrix(ZookeeperCompatibility in your case) from (major, minor, patch).
I could be biased by ClientBuilder::assume_server_version. But I think it might not be what we want to let the caller to construct the compatibility matrix (ZookeeperCompatibility in your case). I don't think users of Curator need such level of customization.
There was a problem hiding this comment.
I'm getting mixed signals here. @eolivelli proposed to remove the constant for ZK 3.5 and let user build the object (and @kezhuw put a thumbs up on this comment), but then @kezhuw is proposing to have user just provide the version and curator do the work, which seems to lean back on my original proposal.
Personally, I'd okay proposing predefined constants but asking the user to provide the full version may be overly complicating things as the only version we currently care about is 3.5 vs 3.6/3.7/3.8 (as permanent watchers seems to be the only feature we are checking for)
There was a problem hiding this comment.
Apologize for this! @laurentgo
I do agree with @eolivelli that enumerate zookeeper version is not good.
as permanent watchers seems to be the only feature we are checking for
I am not sure about this.
asking the user to provide the full version may be overly complicating things as the only version we currently care about is 3.5 vs 3.6/3.7/3.8
Yes, a bit. I guess I have same worry about this. But it is a one for all in api side. That is there will be no new feature toggle apis in future. All features could derive from that version and thus implementation detail.
Given all the candidates(feature enum, feature builder and version class), I prefer to a version class currently.
Any idea ? @eolivelli @tisonkun
There was a problem hiding this comment.
as permanent watchers seems to be the only feature we are checking for
I am not sure about this.
If there are other features (which are not captured by Compatibility class I guess), I'd gladly add those but I don't know enough of Curator/Zookeeper to quickly identify those
There was a problem hiding this comment.
If there are other features (which are not captured by Compatibility class I guess)
This is what I am care about. We have to add new hasXyz each time we find one which I think is not good. But that is fine. We are unlikely to have ten features anyway 😄 .
|
It looks like a newline after the license header was missing |
kezhuw
changed the title
|
Gentle ping. Can I get some guidance on how to make progress on this change? |
|
I'm still interesting in having this issue addressed. Is there anything I can do to to help with this? |
|
+1 to the general idea and +1 to ship it. Better than never! |
|
I cannot restart CI, I am now trying to close/reopen the PR in order to trigger it |
|
@eolivelli anything I can help on my side? |
|
I'm wondering if we can drop some supported versions and reduce our test workflows time consumption ... or we should investigate in each cases. You may check https://issues.apache.org/jira/browse/CURATOR-698 also. |
Add new interface ZookeeperCompatibility to represent server compatibility in addition to the existing Compatibility class (which represents client compatibility).
Enhance CuratorFramework to accept ZookeeperCompatibility instance, allowing user to specify which server version to target (default is LATEST).