ZOOKEEPER-4905: Use create2 only when required

[jjtt]

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.

See:

• Add support for TTL of nodes in ClickHouse Keeper ClickHouse/ClickHouse#55595 (comment)

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.

[kezhuw]

I think the idea is a good in general. But we probably should handle deserialization of response carefully to not break code.

[kezhuw]

I think it is better to flatten these if-else to one level.

[jjtt]

That is an excellent point. Made a fixup commit: 5065b64

[kezhuw]

Create2Response has one more field stat comparing to CreateResponse, so I expected there are test failures.

[jjtt]

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?

[jjtt]

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?

[kezhuw]

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