ibessonov commented on code in PR #6241:
URL: https://github.com/apache/ignite-3/pull/6241#discussion_r2204341140
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##########
@@ -250,16 +281,18 @@ String path() {
public void accept(ConfigShuttle visitor) {
visitor.visit(this);
- for (ConfigNode child : childNodes()) {
- child.accept(visitor);
+ for (NodeReference ref : childNodes()) {
+ for (var node : ref.nodes()) {
Review Comment:
Please don't use `var` in this context, it violates coding guidelines
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##########
@@ -168,18 +176,26 @@ public Collection<ConfigNode> childNodes() {
/**
* Add the child nodes to this node.
*/
- void addChildNodes(Collection<ConfigNode> childNodes) {
+ void addChildReferences(Collection<NodeReference> childNodes) {
assert !flags.contains(Flags.IS_VALUE) : "Value node can't have
children.";
- childNodes.forEach(e -> childNodeMap.put(e.name(), e));
+ childReferences.addAll(childNodes);
+ }
+
+ /**
+ * Add the child nodes to this node.
+ */
+ @TestOnly
Review Comment:
This whole class is test-only, what do we mean by this annotation? Should
this class be in test fixtures instead in such a case? Just asking
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##########
@@ -329,5 +362,31 @@ static class Attributes {
static String NAME = "name";
static String KIND = "kind";
static String CLASS = "class";
+ static String INSTANCE_TYPE = "instanceType";
+ }
+
+ /**
+ * Child node reference.
+ */
Review Comment:
This comment doesn't really explain anything, can you please expand it?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##########
@@ -406,10 +407,8 @@ void testInCompatibleRename() {
}
/**
- * Test scenario. <br>
- * config ver1 has property : prop1 <br>
- * config ver2 has renamed property : prop1 -> prop2 <br>
- * config ver3 has deleted property : prop1, prop2 <br>
+ * Test scenario. <br> config ver1 has property : prop1 <br> config ver2
has renamed property : prop1 -> prop2 <br> config ver3 has
+ * deleted property : prop1, prop2 <br>
Review Comment:
Why did you reformat it? It looks bad now
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeScanner.java:
##########
@@ -94,21 +94,47 @@ public static void scan(ConfigNode currentNode, Class<?>
schemaClass, ScanContex
return;
}
- List<ConfigNode> children = new ArrayList<>();
+ List<NodeReference> children = new ArrayList<>();
+
configurationClasses(schemaClass).stream()
.flatMap(c -> Arrays.stream(c.getDeclaredFields()))
.filter(field -> !Modifier.isStatic(field.getModifiers()))
.sorted(Comparator.comparing(Field::getName)) // Sort for test
stability.
.forEach(field -> {
- ConfigNode node = createNodeForField(currentNode, field);
+ Set<Class<?>> instanceClasses =
context.getPolymorphicInstances(field.getType());
+ List<ConfigNode> fieldNodes = new
ArrayList<>(instanceClasses.size() + 1);
+
+ // Field itself
+ ConfigNode node = createNodeForField(currentNode, field,
field.getType());
+ fieldNodes.add(node);
- children.add(node);
if (!node.isValue()) {
scan(node, field.getType(), context);
}
+
+ // Collect nodes that correspond to polymorphic instances
+ if (!instanceClasses.isEmpty()) {
+ List<Class<?>> instances = new
ArrayList<>(instanceClasses);
+ // Sort classes to make processing stable
+ instances.sort(Comparator.comparing(Class::getName));
+
+ for (Class<?> instanceClass : instances) {
+ ConfigNode instanceTypeNode =
createNodeForField(currentNode, field, instanceClass);
+ fieldNodes.add(instanceTypeNode);
+
+ if (!instanceTypeNode.isValue()) {
+ scan(instanceTypeNode, instanceClass, context);
+ }
+ }
+
+ // Sort subclasses to make data stable.
+
fieldNodes.sort(Comparator.comparing(ConfigNode::className));
Review Comment:
Classes can be renamed, and the rename should not affect configuration
compatibility. What exactly do we mean by stability?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##########
@@ -136,12 +207,13 @@ private static List<ConfigNode> getPath(ConfigNode node) {
/**
* Returns {@code true} if given node is compatible with candidate node,
{@code false} otherwise.
*/
- private static boolean match(ConfigNode node, ConfigNode candidate) {
+ private static boolean match(ConfigNode node, ConfigNode candidate,
@Nullable String instanceType) {
return Objects.equals(candidate.kind(), node.kind())
&& matchNames(candidate, node)
&& validateFlags(candidate, node)
&&
candidate.deletedPrefixes().containsAll(node.deletedPrefixes())
&& (!node.isValue() || Objects.equals(candidate.type(),
node.type())) // Value node types can be changed.
+ && (instanceType == null ||
Objects.equals(candidate.instanceType(), node.instanceType()))
Review Comment:
Could you please explain the meaning behind `instanceType == null` check?
Can we simply remove it?
I believe there's one situation that will be difficult to cover with this
framework. Let's think about it in the future:
- we have a non-polymorphic schema in 3.a
- we make it polymorphic in 3.b, converting on of existing string properties
into a polymorphic type ID
- we add another polymorphic implementation
I believe such thing already happened during development (I don't remember
if it was before or after the release) - it's node finder configuration. Let's
discuss it later
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##########
@@ -465,10 +464,137 @@ public Collection<String> deletedPrefixes() {
assertCompatible(metadataVer2, metadataVer3, new
ComparisonContext(allModules));
}
+ @Test
+ public void testPolymorphicConfigNoChanges() {
+ ConfigNode root = createRoot("root");
+
+ ConfigNode base = createNode(root, "subconfig", "ClassBase");
+ base.addChildNodes(createChild(base, "f1"));
+
+ ConfigNode subclassA = createInstanceNode(root, "subconfig", "ClassA",
"A");
+ subclassA.addChildNodes(createChild(subclassA, "f2"));
+
+ ConfigNode subclassB = createInstanceNode(root, "subconfig", "ClassB",
"B");
+ subclassB.addChildNodes(createChild(subclassB, "f3"));
+
+ root.addChildReferences(List.of(new NodeReference(List.of(base,
subclassA, subclassB))));
+
+ assertCompatible(List.of(root), List.of(root));
+ }
+
+ @Test
+ public void testPolymorphicConfigAddInstance() {
+ ConfigNode root1 = createRoot("root");
+ {
+ ConfigNode base = createNode(root1, "subconfig", "ClassBase");
+ base.addChildNodes(createChild(base, "f1"));
+
+ ConfigNode subclassA = createInstanceNode(root1, "subconfig",
"ClassA", "A");
+ subclassA.addChildNodes(createChild(subclassA, "f2"));
+
+ root1.addChildReferences(List.of(new NodeReference(List.of(base,
subclassA))));
+ }
+
+ ConfigNode root2 = createRoot("root");
+ {
+ ConfigNode base = createNode(root2, "subconfig", "ClassBase");
+ base.addChildNodes(createChild(base, "f1"));
+
+ ConfigNode subclassA = createInstanceNode(root2, "subconfig",
"ClassA", "A");
+ subclassA.addChildNodes(createChild(subclassA, "f2"));
+
+ ConfigNode subclassB = createInstanceNode(root2, "subconfig",
"ClassB", "B");
+ subclassB.addChildNodes(createChild(subclassB, "f3"));
+
+ root2.addChildReferences(List.of(new NodeReference(List.of(base,
subclassA, subclassB))));
+ }
+
+ assertCompatible(List.of(root1), List.of(root2));
+ // Removing should be an incompatible change.
+ assertIncompatible(List.of(root2), List.of(root1));
+ }
+
+ @Test
+ public void testPolymorphicConfigChangeType() {
+ ConfigNode root1 = createRoot("root");
+ {
+ ConfigNode base = createNode(root1, "subconfig", "ClassBase");
+ base.addChildNodes(createChild(base, "f1"));
+
+ ConfigNode subclassA = createInstanceNode(root1, "subconfig",
"ClassA", "A");
+ subclassA.addChildNodes(createChild(subclassA, "f2"));
+
+ root1.addChildReferences(List.of(new NodeReference(List.of(base,
subclassA))));
+ }
+
+ ConfigNode root2 = createRoot("root");
+ {
+ ConfigNode base = createNode(root2, "subconfig", "ClassBase");
+ base.addChildNodes(createChild(base, "f1"));
+
+ ConfigNode subclassA = createInstanceNode(root2, "subconfig",
"ClassA", "B");
+ subclassA.addChildNodes(createChild(subclassA, "f2"));
+
+ root2.addChildReferences(List.of(new NodeReference(List.of(base,
subclassA))));
+ }
+
+ // Changing instance type is always an incompatible change
+ assertIncompatible(List.of(root1), List.of(root2));
+ assertIncompatible(List.of(root2), List.of(root1));
+ }
+
+ @Test
+ public void testPolymorphicConfigAddInstanceField() {
Review Comment:
Let's add one more test:
- you have polymorphic instances with the same field declared in all of
them, for example `int size`
- you decided to more this field into their base class
- this does not violate anything
The opposite must also be true
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
