[GitHub] [zookeeper] eolivelli commented on a change in pull request #1559: ZOOKEEPER-4026: Complete support for Stat objects (and create2) in multi requests

[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