[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1582: IGNITE-18643 Mutability assertions in configuration trees.

["tkalkirill (via GitHub)" <gi...@apache.org>]

tkalkirill commented on code in PR #1582:
URL: https://github.com/apache/ignite-3/pull/1582#discussion_r1091701258


##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java:
##########
@@ -202,12 +205,29 @@ public abstract void construct(
     @Override
     public InnerNode copy() {
         try {
-            return (InnerNode) clone();
+            InnerNode clone = (InnerNode) clone();
+
+            clone.immutable = false;
+
+            return clone;
         } catch (CloneNotSupportedException e) {
             throw new IllegalStateException(e);
         }
     }
 
+    public final void assertMutability() {

Review Comment:
   Missing javaDoc



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -696,6 +700,16 @@ private List<MethodDefinition> addNodeChangeMethod(
 
         BytecodeBlock bytecodeBlock = new BytecodeBlock();
 
+        if (classDef == innerNodeClassDef) {
+            bytecodeBlock.append(changeMtd.getThis().invoke(ASSERT_MUTABILITY_MTD));

Review Comment:
   Show an example of how it will look in byte code.



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1339,6 +1355,8 @@ private void addNodeConstructDefaultMethod(
                 arg("key", String.class)
         ).addException(NoSuchElementException.class);
 
+        constructDfltMtd.getBody().append(constructDfltMtd.getThis().invoke(ASSERT_MUTABILITY_MTD));

Review Comment:
   Same



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConstructableTreeNode.java:
##########
@@ -41,4 +41,11 @@ public interface ConstructableTreeNode {
      * @return Copy of the object.
      */
     ConstructableTreeNode copy();
+
+    /**
+     * Make the node immutable. Following calls of mutating methods on the node should not happen and may result in errors.
+     *
+     * @return {@code true} if node became immutable, {@code false} if it has already been immutable before.
+     */
+    boolean publish();

Review Comment:
   I would like to see a couple of small tests that check the behavior of the method.



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -696,6 +700,16 @@ private List<MethodDefinition> addNodeChangeMethod(
 
         BytecodeBlock bytecodeBlock = new BytecodeBlock();
 
+        if (classDef == innerNodeClassDef) {
+            bytecodeBlock.append(changeMtd.getThis().invoke(ASSERT_MUTABILITY_MTD));
+        } else {
+            bytecodeBlock.append(

Review Comment:
   Same



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/ConstructableTreeNode.java:
##########
@@ -41,4 +41,11 @@ public interface ConstructableTreeNode {
      * @return Copy of the object.
      */
     ConstructableTreeNode copy();
+
+    /**
+     * Make the node immutable. Following calls of mutating methods on the node should not happen and may result in errors.
+     *
+     * @return {@code true} if node became immutable, {@code false} if it has already been immutable before.
+     */
+    boolean publish();

Review Comment:
   For me, the name of the method is a bit confusing, maybe we will rename it to `makeImmutable`.



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1086,6 +1100,8 @@ private void addNodeConstructMethod(
         Variable keyVar = constructMtd.getScope().getVariable("key");
         Variable srcVar = constructMtd.getScope().getVariable("src");
 
+        constructMtd.getBody().append(constructMtd.getThis().invoke(ASSERT_MUTABILITY_MTD));

Review Comment:
   Same



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java:
##########
@@ -484,6 +499,19 @@ public NamedListNode<N> copy() {
         return new NamedListNode<>(this);
     }
 
+    private void assertMutability() {

Review Comment:
   Missing javaDoc, why not `final` ?



##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/InnerNode.java:
##########
@@ -202,12 +205,29 @@ public abstract void construct(
     @Override
     public InnerNode copy() {
         try {
-            return (InnerNode) clone();
+            InnerNode clone = (InnerNode) clone();
+
+            clone.immutable = false;
+
+            return clone;
         } catch (CloneNotSupportedException e) {
             throw new IllegalStateException(e);
         }
     }
 
+    public final void assertMutability() {

Review Comment:
   Could you add a line before each method call to make it easier to read.



-- 
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@ignite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org