You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/12/15 12:41:48 UTC
[GitHub] [zookeeper] ztzg opened a new pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
ztzg opened a new pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559
Without this change, non-TTL/container invocations of `Op.create` invariably use `OpCode.create`, whose response does not include a `Stat` object.
Furthermore, there was no other way of requesting one, and that code path was consequently untested.
The patch introduces an additional `createFlags` parameter which can be used to enable that feature. An invocation with `WANT_STAT` set causes the client to submit an `OpCode.create2`, whose response includes a filled `Stat` object, to the server.
It also completes `OpCode.create2` support for multi transactions, which was problem actually reported by ZOOKEEPER-4026. Without it, such operations are silently downgraded to `OpCode.create`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581185995
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
Review comment:
Done (checking `path` and `acl`).
I had added the checks to the `Op.Create` constructor, but went back to checking at `build()` time, not to change the existing `create(...)` methods—which still do *not* check for `null`—as it may be that some programs accidentally do that. (No error is thrown if the request is never submitted.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581187216
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
+ this.createModeFlag = createModeFlag;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. Either this or {@link #setCreateModeFlag}
+ * must be used before calling {@link #build()}.
+ *
+ * @param createMode the create mode as an enum value
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateMode(CreateMode createMode) {
+ this.createMode = createMode;
Review comment:
Kinda. Passing `null` and not calling `setCreateModeFlag` would be rejected at `build()` time. But passing `null` just for kicks would go undetected.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556478126
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -146,10 +236,33 @@ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode crea
* optional ttl or 0 (createMode must imply a TTL)
*/
public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, long ttl) {
+ return create(path, data, acl, createMode, CreateFlags.DEFAULT, ttl);
+ }
+
+ /**
+ * Constructs a create operation. Arguments are as for the ZooKeeper method of the same name
+ * but adding an optional ttl
+ * @see ZooKeeper#create(String, byte[], java.util.List, CreateMode)
+ *
+ * @param path
+ * the path for the node
+ * @param data
+ * the initial data for the node
+ * @param acl
+ * the acl for the node
+ * @param createMode
+ * specifying whether the node to be created is ephemeral
+ * and/or sequential
+ * @param createFlags
+ * the set of {@link CreateFlags} to apply
+ * @param ttl
+ * optional ttl or 0 (createMode must imply a TTL)
+ */
+ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, Set<CreateFlags> createFlags, long ttl) {
Review comment:
Added a note below: https://github.com/apache/zookeeper/pull/1559#issuecomment-759411279
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r543399622
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/MultiOperationRecord.java
##########
@@ -126,7 +127,10 @@ public void deserialize(InputArchive archive, String tag) throws IOException {
case ZooDefs.OpCode.createContainer:
CreateRequest cr = new CreateRequest();
cr.deserialize(archive, tag);
- add(Op.create(cr.getPath(), cr.getData(), cr.getAcl(), cr.getFlags()));
+ EnumSet<Op.CreateFlags> createFlags = h.getType() == ZooDefs.OpCode.create2
+ ? EnumSet.of(Op.CreateFlags.WANT_STAT)
Review comment:
Can we have a constant even for this case? We are going to reduce memory allocations
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581183467
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
+ this.createModeFlag = createModeFlag;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. Either this or {@link #setCreateModeFlag}
+ * must be used before calling {@link #build()}.
+ *
+ * @param createMode the create mode as an enum value
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateMode(CreateMode createMode) {
+ this.createMode = createMode;
+ return this;
+ }
+
+ /**
+ * Configures whether to include a {@code Stat} object in the
+ * response. Defaults to {@code false}.
+ *
+ * <p>Note that this flag has no effect for TTL or container
+ * nodes, as those always include a {@code Stat} at the
+ * protocol level.
+ *
+ * @param returnStat whether node creation should produce a
+ * {@code Stat} object
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setReturnStat(boolean returnStat) {
+ this.returnStat = returnStat;
+ return this;
+ }
+
+ /**
+ * Sets the TTL for the node. Must be set before calling
+ * {@link #build()} when creating TTL nodes.
+ *
+ * @param ttl the TTL for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setTTL(Long ttl) {
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556466255
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
##########
@@ -443,6 +444,22 @@ public void testCreate(boolean useAsync) throws Exception {
zk.getData("/multi2", false, null);
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateWantStat(boolean useAsync) throws Exception {
+ List<OpResult> results = multi(zk, Arrays.asList(
+ Op.create("/multi0", new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, Op.CreateFlags.DEFAULT),
+ Op.create("/multi1", new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, EnumSet.of(Op.CreateFlags.WANT_STAT)),
+ Op.create("/multi2", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, Op.CreateFlags.DEFAULT),
+ Op.create("/multi3", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, EnumSet.of(Op.CreateFlags.WANT_STAT))),
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-759411279
From https://github.com/apache/zookeeper/pull/1559#discussion_r551931040:
> adding a new factory method is not very awesome from my point of view.
>
> can we take the opportunity to add a fluent Builder for "Op" ?
> also adding "Deprecated" to the other factory methods
I have started looking into this, and have pushed some commits there:
https://github.com/ztzg/zookeeper/commits/ZOOKEEPER-4026-multi-create2-fluent-builder
Making `Op` "fluent" would require… a large number of changes. (Particularly if we deprecate the "old" methods, as we then have to update all the tests to avoid `-Werror` kicking in.)
So while I agree in principle, I would prefer to postpone this cleanup.
Now, I understand that we may not want to introduce four new methods which will be deprecated soon thereafter.
The server-side portion of the fix, however, is valuable independently of the client—so I would like to have it in 3.7.0.
How about:
1. Removing the additional `.create` methods;
2. Making some `Op.Create` constructors package-private;
3. Arranging for tests to use the package-private constructors;
4. Opening a new ticket for the "fluent builder" client improvements;
5. Contributing an adapted version of https://github.com/ztzg/zookeeper/commits/ZOOKEEPER-4026-multi-create2-fluent-builder as a WIP PR for that ticket.
This means that Java applications would be unable to exercise the new code path in the meantime, but it would at least be tested and fix the problem for Kazoo/other clients.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581121234
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
Review comment:
It is better to perform validation on build() when possible
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581187216
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
+ this.createModeFlag = createModeFlag;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. Either this or {@link #setCreateModeFlag}
+ * must be used before calling {@link #build()}.
+ *
+ * @param createMode the create mode as an enum value
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateMode(CreateMode createMode) {
+ this.createMode = createMode;
Review comment:
Kinda. Passing `null` and not calling `setCreateModeFlag` would be validated at `build()` time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581183364
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556595999
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -146,10 +236,33 @@ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode crea
* optional ttl or 0 (createMode must imply a TTL)
*/
public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, long ttl) {
+ return create(path, data, acl, createMode, CreateFlags.DEFAULT, ttl);
+ }
+
+ /**
+ * Constructs a create operation. Arguments are as for the ZooKeeper method of the same name
+ * but adding an optional ttl
+ * @see ZooKeeper#create(String, byte[], java.util.List, CreateMode)
+ *
+ * @param path
+ * the path for the node
+ * @param data
+ * the initial data for the node
+ * @param acl
+ * the acl for the node
+ * @param createMode
+ * specifying whether the node to be created is ephemeral
+ * and/or sequential
+ * @param createFlags
+ * the set of {@link CreateFlags} to apply
+ * @param ttl
+ * optional ttl or 0 (createMode must imply a TTL)
+ */
+ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, Set<CreateFlags> createFlags, long ttl) {
Review comment:
> I am leaning toward adding the Builder support without marking the methods as Deprecated (in order to not change tests, we have a few important big forks and changing lots of tests will be a pain for every one).
>
> So adding the Builder and not adding this new overloaded method here.
Right. That's what I did in this other branch:
https://github.com/ztzg/zookeeper/commits/ZOOKEEPER-4026-multi-create2-fluent-builder
In particular, here is the builder for `Create` and `CreateTTL`:
https://github.com/ztzg/zookeeper/commit/d7bab23d8faf2f364a8bbe32a2c15170030e4d40#diff-d6e7807a04d07a070ee3da75ccd793bd973b5af0ee5597f0969a0a7721d0200fR376
> I am not sure that adding the Builder is about adding much code.
There are *six* direct subclasses of `Op`, though: https://javadoc.io/doc/org.apache.zookeeper/zookeeper/latest/org/apache/zookeeper/Op.html . (The only indirect one is `CreateTTL`, and it is already handed.)
But looking closer, the others do not warrant introducing a `*Builder`, as they don't have the same variability in parameters. Would you be okay with the asymmetry? (I.e., only introducing a `CreateBuilder`, and leaving the rest as-is.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556714736
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -146,10 +236,33 @@ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode crea
* optional ttl or 0 (createMode must imply a TTL)
*/
public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, long ttl) {
+ return create(path, data, acl, createMode, CreateFlags.DEFAULT, ttl);
+ }
+
+ /**
+ * Constructs a create operation. Arguments are as for the ZooKeeper method of the same name
+ * but adding an optional ttl
+ * @see ZooKeeper#create(String, byte[], java.util.List, CreateMode)
+ *
+ * @param path
+ * the path for the node
+ * @param data
+ * the initial data for the node
+ * @param acl
+ * the acl for the node
+ * @param createMode
+ * specifying whether the node to be created is ephemeral
+ * and/or sequential
+ * @param createFlags
+ * the set of {@link CreateFlags} to apply
+ * @param ttl
+ * optional ttl or 0 (createMode must imply a TTL)
+ */
+ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, Set<CreateFlags> createFlags, long ttl) {
Review comment:
Okay—the version I just pushed implements the "fluent builder" pattern for `Create`, and only uses a simple `boolean` for the new feature, as pondered in https://github.com/apache/zookeeper/pull/1559#discussion_r556586446.
I find the result simpler and easier to read. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581117782
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
Review comment:
Yes, we do.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581117782
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
Review comment:
Yes, we do.
(I mean, `data` can be `null` for create requests. Would you suggest preventing setting a `null` data on the builder, forcing the caller to use an `if`?)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581186298
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
Review comment:
*Not* changed, and explicitly documented as accepting `null`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556482659
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -54,6 +56,35 @@
READ
}
+ /**
+ * Optional flags for the
+ * {@link Op#create(String, byte[], List, CreateMode, EnumSet)}
+ * and similar operations.
+ */
+ public enum CreateFlags {
+ /**
+ * Requests that a {@code Stat} object be included in the
+ * response.
+ *
+ * This is semantically equivalent to passing a non-{@code null}
+ * {@code stat} parameter to
+ * {@link ZooKeeper#create(String, byte[], List, CreateMode, Stat)}.
+ */
+ WITH_STAT;
Review comment:
RETURN_STAT ?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -146,10 +236,33 @@ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode crea
* optional ttl or 0 (createMode must imply a TTL)
*/
public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, long ttl) {
+ return create(path, data, acl, createMode, CreateFlags.DEFAULT, ttl);
+ }
+
+ /**
+ * Constructs a create operation. Arguments are as for the ZooKeeper method of the same name
+ * but adding an optional ttl
+ * @see ZooKeeper#create(String, byte[], java.util.List, CreateMode)
+ *
+ * @param path
+ * the path for the node
+ * @param data
+ * the initial data for the node
+ * @param acl
+ * the acl for the node
+ * @param createMode
+ * specifying whether the node to be created is ephemeral
+ * and/or sequential
+ * @param createFlags
+ * the set of {@link CreateFlags} to apply
+ * @param ttl
+ * optional ttl or 0 (createMode must imply a TTL)
+ */
+ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, Set<CreateFlags> createFlags, long ttl) {
Review comment:
I am leaning toward adding the Builder support without marking the methods as Deprecated (in order to not change tests, we have a few important big forks and changing lots of tests will be a pain for every one).
So adding the Builder and not adding this new overloaded method here.
I am not sure that adding the Builder is about adding much code.
cc @hanm @lvfangmin
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r552172317
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -1001,6 +1001,7 @@ public ProcessTxnResult processTxn(TxnHeader header, Record txn, boolean isSubTx
Record record = null;
switch (subtxn.getType()) {
case OpCode.create:
+ case OpCode.create2:
Review comment:
I will double-check, but from the top of my head: no error, just missing stat data. (Charles-Henri, who reported ZOOKEEPER-4026, discovered the problem while adding `multi` support to Kazoo. The "2" in front of the request just fell off :)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556466384
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -54,6 +56,29 @@
READ
}
+ /**
+ * Optional flags for the
+ * {@link Op#create(String, byte[], List, CreateMode, EnumSet)}
+ * and similar operations.
+ */
+ public enum CreateFlags {
+ /**
+ * Requests that a {@code Stat} object be included in the
+ * response.
+ *
+ * This is semantically equivalent to passing a non-{@code null}
+ * {@var stat} parameter to
+ * {@link ZooKeeper#create(String, byte[], List, CreateMode, Stat)}.
+ */
+ WANT_STAT;
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r543642732
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/MultiOperationRecord.java
##########
@@ -126,7 +127,10 @@ public void deserialize(InputArchive archive, String tag) throws IOException {
case ZooDefs.OpCode.createContainer:
CreateRequest cr = new CreateRequest();
cr.deserialize(archive, tag);
- add(Op.create(cr.getPath(), cr.getData(), cr.getAcl(), cr.getFlags()));
+ EnumSet<Op.CreateFlags> createFlags = h.getType() == ZooDefs.OpCode.create2
+ ? EnumSet.of(Op.CreateFlags.WANT_STAT)
Review comment:
Done.
Adding that odd case to the visible API felt a bit awkward, so I have attached it to the `MultiOperationRecord` implementation. (Of course, the "odd case"-ness is debatable since we only have a single flag :) Let me know if you'd prefer to have it in the enum. (In which case I would also appreciate a suggestion for a non-clashing name!)
I have also wrapped both `EnumSet`s with `Collections.unmodifiableSet`, as the former are in fact mutable.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581113061
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
Review comment:
Yes, the intent was to allow (re)setting these properties to `null` (which is their default value), otherwise there is no way to put the object back into its default state.
(As I suppose you have noticed, `null` is notably used as a marker for `createMode` and `createModeFlag`, which are exclusive.)
But I can switch it to `int`; I don't expect that many people will want to reuse `CreateBuilder` instances.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-784328411
> I left some final feedback
(Mostly) addressed. What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581186498
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
Review comment:
Validated at `build()` time.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-867403508
Hi @eolivelli,
> > I left some final feedback
>
> (Mostly) addressed. What do you think?
I don't have any points regarding this. Could you please take another look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-888986339
> I am fine with committing it to master branch, but I am not sure it fits a minor release because it is an API change
Agree, and I would be glad to see it in `master`.
Unfortunately, I was not able to fix the server-side bug without changing (and later "improving") some of the APIs, as the `MultiOperationRecord` class relies on `Op.Create`. I could try and come up with a minimally invasive patch for 3.7, but it may be difficult and/or ugly, and probably isn't worth it as discovering the issue took so long 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: notifications-unsubscribe@zookeeper.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581117530
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
Review comment:
The idea was to perform validation at `build()` time—but it is true that `path` and `acl` are not, in fact, validated by the `Create`/`CreateTTL` constructors. I suppose that should be fixed.
Or are you suggesting that a check should be added at *this* point? (That would match your `int`/`long` comment, and correspond to a "properties must be set once, and must be set right" model—which is not what I was initially going 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556735283
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -1001,6 +1001,7 @@ public ProcessTxnResult processTxn(TxnHeader header, Record txn, boolean isSubTx
Record record = null;
switch (subtxn.getType()) {
case OpCode.create:
+ case OpCode.create2:
Review comment:
I have double-checked this, and my recollection was correct: a new client requesting a `Stat` from 3.5, 3.6 or current `master` receives a response, but it is missing `Stat` data.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r556586446
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -54,6 +56,35 @@
READ
}
+ /**
+ * Optional flags for the
+ * {@link Op#create(String, byte[], List, CreateMode, EnumSet)}
+ * and similar operations.
+ */
+ public enum CreateFlags {
+ /**
+ * Requests that a {@code Stat} object be included in the
+ * response.
+ *
+ * This is semantically equivalent to passing a non-{@code null}
+ * {@code stat} parameter to
+ * {@link ZooKeeper#create(String, byte[], List, CreateMode, Stat)}.
+ */
+ WITH_STAT;
Review comment:
You suggested `WITH_STAT` or `RETURN_STAT`, so… I used both :)
More seriously, there is already a `RETURN_STAT` a few lines below. `WITH_STAT` is the enumeration value, and `RETURN_STAT` the constant `Set`.
I'm not very happy with that. But I suppose we actually don't need a set of flags if we use the builder approach; individual booleans are fine if the setters are named.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-784235346
Hi @eolivelli, I believe I have addressed your feedback; PTAL.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-754877418
> I left some feedback, please take a look
I have noted your points; thanks! I will look closer and respin the PR soon.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r581074879
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
Review comment:
why "Integer" ?
is it expected to pass "null" ?
I am fine with having `Integer` as internal value, but maybe here in this point we should have only `int`
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
+ this.createModeFlag = createModeFlag;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. Either this or {@link #setCreateModeFlag}
+ * must be used before calling {@link #build()}.
+ *
+ * @param createMode the create mode as an enum value
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateMode(CreateMode createMode) {
+ this.createMode = createMode;
+ return this;
+ }
+
+ /**
+ * Configures whether to include a {@code Stat} object in the
+ * response. Defaults to {@code false}.
+ *
+ * <p>Note that this flag has no effect for TTL or container
+ * nodes, as those always include a {@code Stat} at the
+ * protocol level.
+ *
+ * @param returnStat whether node creation should produce a
+ * {@code Stat} object
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setReturnStat(boolean returnStat) {
+ this.returnStat = returnStat;
+ return this;
+ }
+
+ /**
+ * Sets the TTL for the node. Must be set before calling
+ * {@link #build()} when creating TTL nodes.
+ *
+ * @param ttl the TTL for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setTTL(Long ttl) {
Review comment:
the same here, we should have `long`
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
Review comment:
are we allowing a `null` value here ?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
Review comment:
are we allowing a `null` value here ?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
+ return this;
+ }
+
+ /**
+ * Sets the initial data for the node, or {@code null}.
+ *
+ * @param data the initial data for the node
+ * @return the builder, for chaining.
+ */
+ @SuppressFBWarnings({"EI_EXPOSE_REP", "EI_EXPOSE_REP2"})
+ public CreateBuilder setData(byte[] data) {
+ this.data = data;
+ return this;
+ }
+
+ /**
+ * Sets the acl for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param acl the acl for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setACL(List<ACL> acl) {
+ this.acl = acl;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. using the integer encoding. Either this
+ * or {@link #setCreateMode} must be used before calling
+ * {@link #build()}.
+ *
+ * @param createModeFlag the create mode encoded as an integer
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateModeFlag(Integer createModeFlag) {
+ this.createModeFlag = createModeFlag;
+ return this;
+ }
+
+ /**
+ * Specifyies whether the node to be created is ephemeral,
+ * sequential, etc. Either this or {@link #setCreateModeFlag}
+ * must be used before calling {@link #build()}.
+ *
+ * @param createMode the create mode as an enum value
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setCreateMode(CreateMode createMode) {
+ this.createMode = createMode;
Review comment:
are we allowing a `null` value here ?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -253,6 +254,142 @@ void validate() throws KeeperException {
PathUtils.validatePath(path);
}
+ /**
+ * "Fluent builder" for {@link Create} operations.
+ */
+ public static class CreateBuilder {
+ private String path;
+ private byte[] data;
+ private List<ACL> acl;
+ private Integer createModeFlag;
+ private CreateMode createMode;
+ private boolean returnStat = false;
+ private Long ttl;
+
+ /**
+ * Sets the path for the node. Must be set before calling
+ * {@link #build()}.
+ *
+ * @param path the path for the node
+ * @return the builder, for chaining.
+ */
+ public CreateBuilder setPath(String path) {
+ this.path = path;
Review comment:
are we allowing a `null` value here ?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg edited a comment on pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
ztzg edited a comment on pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#issuecomment-867403508
Hi @eolivelli,
> > I left some final feedback
>
> (Mostly) addressed. What do you think?
I don't have any open points regarding this. Rebasing the PR. Could you please take another look?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1559:
URL: https://github.com/apache/zookeeper/pull/1559#discussion_r551929517
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/MultiOperationTest.java
##########
@@ -443,6 +444,22 @@ public void testCreate(boolean useAsync) throws Exception {
zk.getData("/multi2", false, null);
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = {true, false})
+ public void testCreateWantStat(boolean useAsync) throws Exception {
+ List<OpResult> results = multi(zk, Arrays.asList(
+ Op.create("/multi0", new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, Op.CreateFlags.DEFAULT),
+ Op.create("/multi1", new byte[0], Ids.OPEN_ACL_UNSAFE, CreateMode.PERSISTENT, EnumSet.of(Op.CreateFlags.WANT_STAT)),
+ Op.create("/multi2", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, Op.CreateFlags.DEFAULT),
+ Op.create("/multi3", new byte[0], Ids.OPEN_ACL_UNSAFE, 0, EnumSet.of(Op.CreateFlags.WANT_STAT))),
Review comment:
Probably this looks like boilerplate
`EnumSet.of(Op.CreateFlags.WANT_STAT))`
Can we create a constant like Op.CreateFlags.DEFAULT ?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/DataTree.java
##########
@@ -1001,6 +1001,7 @@ public ProcessTxnResult processTxn(TxnHeader header, Record txn, boolean isSubTx
Record record = null;
switch (subtxn.getType()) {
case OpCode.create:
+ case OpCode.create2:
Review comment:
this change is interesting here, because it means that this feature will be supported only when the server is 3.7+
which error will be reported to a 3.7 client that talks to a 3.6/3.5 server?
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -146,10 +236,33 @@ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode crea
* optional ttl or 0 (createMode must imply a TTL)
*/
public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, long ttl) {
+ return create(path, data, acl, createMode, CreateFlags.DEFAULT, ttl);
+ }
+
+ /**
+ * Constructs a create operation. Arguments are as for the ZooKeeper method of the same name
+ * but adding an optional ttl
+ * @see ZooKeeper#create(String, byte[], java.util.List, CreateMode)
+ *
+ * @param path
+ * the path for the node
+ * @param data
+ * the initial data for the node
+ * @param acl
+ * the acl for the node
+ * @param createMode
+ * specifying whether the node to be created is ephemeral
+ * and/or sequential
+ * @param createFlags
+ * the set of {@link CreateFlags} to apply
+ * @param ttl
+ * optional ttl or 0 (createMode must imply a TTL)
+ */
+ public static Op create(String path, byte[] data, List<ACL> acl, CreateMode createMode, Set<CreateFlags> createFlags, long ttl) {
Review comment:
adding a new factory method is not very awesome from my point of view.
can we take the opportunity to add a fluent Builder for "Op" ?
also adding "Deprecated" to the other factory methods
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/Op.java
##########
@@ -54,6 +56,29 @@
READ
}
+ /**
+ * Optional flags for the
+ * {@link Op#create(String, byte[], List, CreateMode, EnumSet)}
+ * and similar operations.
+ */
+ public enum CreateFlags {
+ /**
+ * Requests that a {@code Stat} object be included in the
+ * response.
+ *
+ * This is semantically equivalent to passing a non-{@code null}
+ * {@var stat} parameter to
+ * {@link ZooKeeper#create(String, byte[], List, CreateMode, Stat)}.
+ */
+ WANT_STAT;
Review comment:
I am not sure this is a good name.
what about `RETURN_STAT` or `WITH_STAT`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org