You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2023/01/17 13:54:00 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request, #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
ibessonov opened a new pull request, #1540:
URL: https://github.com/apache/ignite-3/pull/1540
…ent access to "external" configuration. New "change" API introduced bot in registry and in generated Change interfaces.
https://issues.apache.org/jira/browse/IGNITE-18516
--
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
[GitHub] [ignite-3] tkalkirill commented on a diff in pull request #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on code in PR #1540:
URL: https://github.com/apache/ignite-3/pull/1540#discussion_r1073333953
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -518,6 +518,14 @@ private void createPojoBindings(
}
changeClsBuilder.addMethod(changeMtdBuilder.build());
+
+ if (valAnnotation == null) {
Review Comment:
Please add an example of what it will be and describe what it is for.
I also don't see a test for this logic.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -345,15 +346,15 @@ private ClassDefinition createNodeClass() {
}
// Add change methods.
- MethodDefinition changeMtd0 = addNodeChangeMethod(
+ List<MethodDefinition> changeMethods = addNodeChangeMethod(
innerNodeClassDef,
schemaField,
changeMtd -> getThisFieldCode(changeMtd, fieldDef),
(changeMtd, newValue) -> setThisFieldCode(changeMtd, newValue, fieldDef),
null
);
- addNodeChangeBridgeMethod(innerNodeClassDef, changeClassName(schemaField.getDeclaringClass()), changeMtd0);
+ addNodeChangeBridgeMethod(innerNodeClassDef, changeClassName(schemaField.getDeclaringClass()), changeMethods.get(0));
Review Comment:
Please add comment why `get(0)`.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1610,15 +1646,15 @@ private ClassDefinition createPolymorphicExtensionNodeClass(
viewMtd -> getThisFieldCode(viewMtd, parentInnerNodeFieldDef, polymorphicTypeIdFieldDef)
);
- MethodDefinition changeMtd0 = addNodeChangeMethod(
+ List<MethodDefinition> changeMethods = addNodeChangeMethod(
classDef,
polymorphicField,
changeMtd -> getThisFieldCode(changeMtd, parentInnerNodeFieldDef, polymorphicFieldDef),
(changeMtd, newValue) -> setThisFieldCode(changeMtd, newValue, parentInnerNodeFieldDef, polymorphicFieldDef),
changeMtd -> getThisFieldCode(changeMtd, parentInnerNodeFieldDef, polymorphicTypeIdFieldDef)
);
- addNodeChangeBridgeMethod(classDef, polymorphicExtensionClassInfo.changeClassName, changeMtd0);
+ addNodeChangeBridgeMethod(classDef, polymorphicExtensionClassInfo.changeClassName, changeMethods.get(0));
Review Comment:
Same.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -1586,15 +1622,15 @@ private ClassDefinition createPolymorphicExtensionNodeClass(
continue;
}
- MethodDefinition changeMtd0 = addNodeChangeMethod(
+ List<MethodDefinition> changeMethods = addNodeChangeMethod(
classDef,
schemaField,
changeMtd -> getThisFieldCode(changeMtd, parentInnerNodeFieldDef, schemaFieldDef),
(changeMtd, newValue) -> setThisFieldCode(changeMtd, newValue, parentInnerNodeFieldDef, schemaFieldDef),
null
);
- addNodeChangeBridgeMethod(classDef, schemaClassInfo.changeClassName, changeMtd0);
+ addNodeChangeBridgeMethod(classDef, schemaClassInfo.changeClassName, changeMethods.get(0));
Review Comment:
Same about `get(0)`.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -731,56 +718,105 @@ private MethodDefinition addNodeChangeMethod(
// this.field = newValue;
bytecodeBlock.append(setFieldCodeFun.apply(changeMtd, newValue));
} else {
- BytecodeExpression newValue;
+ shortChangeMtd = createShortChangeMethod(classDef, schemaField, getFieldCodeFun, setFieldCodeFun, getPolymorphicTypeIdFieldFun);
- if (isConfigValue(schemaField)) {
- // newValue = (this.field == null) ? new ValueNode() : (ValueNode)this.field.copy();
- newValue = cgen.newOrCopyNodeField(schemaField, getFieldCodeFun.apply(changeMtd));
- } else {
- assert isNamedConfigValue(schemaField) : schemaField;
+ // change.accept(this.field); OR change.accept(this.field.specificNode());
+ bytecodeBlock.append(changeVar.invoke(ACCEPT, changeMtd.getThis().invoke(shortChangeMtd, List.of())));
+ }
- // newValue = (ValueNode)this.field.copy();
- newValue = cgen.copyNodeField(schemaField, getFieldCodeFun.apply(changeMtd));
- }
+ // return this;
+ bytecodeBlock.append(changeMtd.getThis()).retObject();
- // this.field = newValue;
- bytecodeBlock.append(setFieldCodeFun.apply(changeMtd, newValue));
+ enrichWithPolymorphicTypeCheck(schemaField, getPolymorphicTypeIdFieldFun, changeMtd, bytecodeBlock);
- // this.field;
- BytecodeExpression getFieldCode = getFieldCodeFun.apply(changeMtd);
+ if (shortChangeMtd == null) {
+ return List.of(changeMtd);
+ }
- if (isPolymorphicConfig(schemaFieldType) && isConfigValue(schemaField)) {
- // this.field.specificNode();
- getFieldCode = getFieldCode.invoke(SPECIFIC_NODE_MTD);
- }
+ return List.of(changeMtd, shortChangeMtd);
+ }
- // change.accept(this.field); OR change.accept(this.field.specificNode());
- bytecodeBlock.append(changeVar.invoke(ACCEPT, getFieldCode));
+ /**
+ * Creates a "short" method to return a changed field instance. Name is the same as for {@code changeFoo(Consumer<Foo> change)"}.
Review Comment:
Please add an example of what it would look like.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -272,6 +275,39 @@ public CompletableFuture<Void> change(ConfigurationSource changesSrc) {
return changer.change(changesSrc);
}
+ /**
+ * Change configuration. Gives the possibility to atomically update several root trees.
+ *
+ * @param change Closure that would consume a mutable super root instance.
+ * @return Future that is completed on change completion.
+ */
+ public CompletableFuture<Void> change(Consumer<SuperRootChange> change) {
+ return change(new ConfigurationSource() {
+ @Override
+ public void descend(ConstructableTreeNode node) {
+ assert node instanceof SuperRoot : "Descending always starts with super root.";
Review Comment:
```suggestion
assert node instanceof SuperRoot : "Descending always starts with super root: " + node ;
```
--
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
[GitHub] [ignite-3] ibessonov merged pull request #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
Posted by GitBox <gi...@apache.org>.
ibessonov merged PR #1540:
URL: https://github.com/apache/ignite-3/pull/1540
--
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
[GitHub] [ignite-3] tkalkirill commented on pull request #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
Posted by GitBox <gi...@apache.org>.
tkalkirill commented on PR #1540:
URL: https://github.com/apache/ignite-3/pull/1540#issuecomment-1386866043
Also please update `modules\configuration\README.md` due to these changes.
If I'm not mistaken, I didn't find tests that would test new "changes" methods for a specific configuration for example `org.apache.ignite.internal.schema.configuration.TableChange#changeDataStorage(`).
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1540:
URL: https://github.com/apache/ignite-3/pull/1540#discussion_r1073517653
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/asm/InnerNodeAsmGenerator.java:
##########
@@ -345,15 +346,15 @@ private ClassDefinition createNodeClass() {
}
// Add change methods.
- MethodDefinition changeMtd0 = addNodeChangeMethod(
+ List<MethodDefinition> changeMethods = addNodeChangeMethod(
innerNodeClassDef,
schemaField,
changeMtd -> getThisFieldCode(changeMtd, fieldDef),
(changeMtd, newValue) -> setThisFieldCode(changeMtd, newValue, fieldDef),
null
);
- addNodeChangeBridgeMethod(innerNodeClassDef, changeClassName(schemaField.getDeclaringClass()), changeMtd0);
+ addNodeChangeBridgeMethod(innerNodeClassDef, changeClassName(schemaField.getDeclaringClass()), changeMethods.get(0));
Review Comment:
ok
--
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
[GitHub] [ignite-3] ibessonov commented on a diff in pull request #1540: IGNITE-18516 "Change closure" semantic fixed when it comes to concurr…
Posted by GitBox <gi...@apache.org>.
ibessonov commented on code in PR #1540:
URL: https://github.com/apache/ignite-3/pull/1540#discussion_r1073517061
##########
modules/configuration-annotation-processor/src/main/java/org/apache/ignite/internal/configuration/processor/ConfigurationProcessor.java:
##########
@@ -518,6 +518,14 @@ private void createPojoBindings(
}
changeClsBuilder.addMethod(changeMtdBuilder.build());
+
+ if (valAnnotation == null) {
Review Comment:
I added tests for it. I'll add a comment.
--
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