ZOOKEEPER-4905: Use create2 only when required #2230
Conversation
If the client is not requesting stat, then the basic create operation is enough. This makes the client library compatible with servers that do not implement all ZooKeeper protocol features. Concretely, ClickHouse keeper does not implement create2.
kezhuw
changed the title
| if (createMode.isTTL()) { | ||
| h.setType(ZooDefs.OpCode.createTTL); | ||
| } else { | ||
| h.setType(createMode.isContainer() ? ZooDefs.OpCode.createContainer : ZooDefs.OpCode.create2); | ||
| if (createMode.isContainer()) { |
There was a problem hiding this comment.
I think it is better to flatten these if-else to one level.
There was a problem hiding this comment.
That is an excellent point. Made a fixup commit: 5065b64
| @@ -1490,7 +1490,7 @@ public String create( | |||
| final String serverPath = prependChroot(clientPath); | |||
|
|
|||
| RequestHeader h = new RequestHeader(); | |||
| setCreateHeader(createMode, h); | |||
| setCreateHeader(createMode, h, stat != null); | |||
| Create2Response response = new Create2Response(); | |||
There was a problem hiding this comment.
Create2Response has one more field stat comparing to CreateResponse, so I expected there are test failures.
There was a problem hiding this comment.
Yes, there are test failures (I was lazy and hoping the CI would run the tests)
For example org.apache.zookeeper.test.CreateTest#testCreateWithNullStat is failing with a null value in org.apache.zookeeper.proto.Create2Response#deserialize.
More precisely it's a null stat that is being deserialized? org.apache.zookeeper.data.Stat#deserialize
Is this something like the server is sending a CreateResponse and the client is trying to parse it as a Create2Response?
There was a problem hiding this comment.
After staring at the code for a while I now think that my suggestion here is not a realistic way forward.
Something like separate create() methods for using the different create operations and then maybe a single higher level method that chooses which one to use?
There was a problem hiding this comment.
--- a/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
+++ b/zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
@@ -1482,6 +1482,9 @@ public String create(
CreateMode createMode,
Stat stat,
long ttl) throws KeeperException, InterruptedException {
+ if (stat == null && ttl == -1) {
+ return create(path, data, acl, createMode);
+ }
final String clientPath = path;
PathUtils.validatePath(clientPath, createMode.isSequential());
EphemeralType.validateTTL(createMode, ttl);How about this ?
I think it is a good to provide generic(a.k.a. full parameters) method to fallback protocol code for compatibilities either with 3.4.x or ClickHouse Keeper if the code space is restricted. Any thoughts ? cc @tisonkun @eolivelli @cnauroth
I saw getChildren2, it is similar. If we are going this way, we should do it for getChildren also. I thought it is similar to include_data in kazoo's create and get_children.
I do think clients have option to wrap themselves if they want such.
If the client is not requesting stat, then the basic
createoperation is enough.This makes the client library compatible with servers that do not implement all ZooKeeper protocol features. Concretely, ClickHouse Keeper does not implement
create2.See:
Background
I was testing if an application that uses Apache Curator to talk to a ZooKeeper could work with a ClickHouse Keeper instead. This would avoid having to run both ZooKeeper and ClickHouse Keeper in our environment.
With a patch like this I was able to get things to work and I couldn't find any other way to instruct the client library to avoid using
create2.This is mostly a compatibility issue with a third party, so arguably should be fixed there, but since I'm much more familiar with Java than C++ I thought I'd try changing the client library here.
I'm hoping someone more familiar with the codebase can take a look and comment if this is sensible or not.