You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@curator.apache.org by "laurentgo (via GitHub)" <gi...@apache.org> on 2024/03/11 17:59:21 UTC
[PR] CURATOR-709. Add server compatibility check support [curator]
laurentgo opened a new pull request, #497:
URL: https://github.com/apache/curator/pull/497
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).
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2082332784
I cannot restart CI, I am now trying to close/reopen the PR in order to trigger it
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1523613127
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun merged PR #497:
URL: https://github.com/apache/curator/pull/497
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1520199762
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1523578958
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2088855773
@eolivelli anything I can help on my side?
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1522754700
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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](https://docs.rs/zookeeper-client/0.6.2/zookeeper_client/struct.ClientBuilder.html#method.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.
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2040176461
I'm still interesting in having this issue addressed. Is there anything I can do to to help with this?
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "eolivelli (via GitHub)" <gi...@apache.org>.
eolivelli closed pull request #497: CURATOR-704. Add server compatibility check support
URL: https://github.com/apache/curator/pull/497
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1520792175
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review Comment:
I guess we probably are pursuing something similar to [`ClientBuilder::assume_server_version`](https://docs.rs/zookeeper-client/0.6.2/zookeeper_client/struct.ClientBuilder.html#method.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.
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-1989161883
@eolivelli can you help me to understand the CI / Check failure? It seems to be a license check but AFAIK ZookeeperCompatibility` class does have the ASF header.
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2012947014
Gentle ping. Can I get some guidance on how to make progress on this change?
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2082025751
+1 to the general idea and +1 to ship it. Better than never!
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1521854920
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-709. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1520236131
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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?
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "laurentgo (via GitHub)" <gi...@apache.org>.
laurentgo commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1523519824
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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)
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "kezhuw (via GitHub)" <gi...@apache.org>.
kezhuw commented on code in PR #497:
URL: https://github.com/apache/curator/pull/497#discussion_r1582621155
##########
curator-client/src/main/java/org/apache/curator/utils/ZookeeperCompatibility.java:
##########
@@ -0,0 +1,47 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.curator.utils;
+
+/**
+ * Describe feature supports based on server compatibility (as opposed to {@code Compatibility}
+ * which represents client compatibility.
+ */
+public interface ZookeeperCompatibility {
+ public enum Version implements ZookeeperCompatibility {
+ VERSION_3_5(false),
Review 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 😄 .
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] CURATOR-704. Add server compatibility check support [curator]
Posted by "tisonkun (via GitHub)" <gi...@apache.org>.
tisonkun commented on PR #497:
URL: https://github.com/apache/curator/pull/497#issuecomment-2090189691
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.
--
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: commits-unsubscribe@curator.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org