Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ZOOKEEPER-4905: Use create2 only when required #2230

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+8 −4
Open

ZOOKEEPER-4905: Use create2 only when required #2230

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4e039db
Use create2 only when required
jjtt Mar 5, 2025
5065b64
fixup! Use create2 only when required
jjtt Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions zookeeper-server/src/main/java/org/apache/zookeeper/ZooKeeper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Record record = makeCreateRecord(createMode, serverPath, data, acl, ttl);
ReplyHeader r = cnxn.submitRequest(h, record, response, null);
Expand All @@ -1503,11 +1503,19 @@ public String create(
return chroot.strip(response.getPath());
}

private void setCreateHeader(CreateMode createMode, RequestHeader h) {
private void setCreateHeader(CreateMode createMode, RequestHeader h, boolean requestStat) {
if (createMode.isTTL()) {
h.setType(ZooDefs.OpCode.createTTL);
} else {
h.setType(createMode.isContainer() ? ZooDefs.OpCode.createContainer : ZooDefs.OpCode.create2);
if (createMode.isContainer()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

h.setType(ZooDefs.OpCode.createContainer);
} else {
if (requestStat) {
h.setType(ZooDefs.OpCode.create2);
} else {
h.setType(ZooDefs.OpCode.create);
}
}
}
}

Expand Down Expand Up @@ -1599,7 +1607,7 @@ public void create(
cb = chroot.interceptCallback(cb);

RequestHeader h = new RequestHeader();
setCreateHeader(createMode, h);
setCreateHeader(createMode, h, false);
ReplyHeader r = new ReplyHeader();
Create2Response response = new Create2Response();
Record record = makeCreateRecord(createMode, serverPath, data, acl, ttl);
Expand Down
Loading