Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov merged PR #6325: URL: https://github.com/apache/ignite-3/pull/6325 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
ibessonov commented on code in PR #6325:
URL: https://github.com/apache/ignite-3/pull/6325#discussion_r2238986734
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeScanner.java:
##
@@ -83,33 +81,75 @@ public class ConfigurationTreeScanner {
* @param context The context containing dependency information.
*/
public static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context) {
+scan(currentNode, schemaClass, context, Set.of());
+}
+
+private static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context, Set skipFields) {
assert schemaClass != null &&
schemaClass.getName().startsWith("org.apache.ignite");
Collection> extensions = context.getExtensions(schemaClass);
if (!extensions.isEmpty()) {
extensions.stream()
.sorted(Comparator.comparing(Class::getName)) // Sort for
test stability.
-.forEach(ext -> scan(currentNode, ext, context));
+.forEach(ext -> scan(currentNode, ext, context,
skipFields));
return;
}
-List children = new ArrayList<>();
+Map>> instancesPerField = new HashMap<>();
+String[] defaultInstanceId = new String[1];
+
+// Non-polymorphic fields
configurationClasses(schemaClass).stream()
.flatMap(c -> Arrays.stream(c.getDeclaredFields()))
.filter(field -> !Modifier.isStatic(field.getModifiers()))
+.filter(field -> !skipFields.contains(field.getName()))
.sorted(Comparator.comparing(Field::getName)) // Sort for test
stability.
.forEach(field -> {
-ConfigNode node = createNodeForField(currentNode, field);
+Class type = field.getType();
+Set> instanceClasses =
context.getPolymorphicInstances(type);
+
+// Field itself
+ConfigNode node = createNodeForField(currentNode, field,
type);
+
+if (instanceClasses.isEmpty()) {
+// Single node
+if (!node.isValue()) {
+scan(node, type, context, skipFields);
+}
-children.add(node);
-if (!node.isValue()) {
-scan(node, field.getType(), context);
+currentNode.addChildNodes(List.of(node));
+} else {
+defaultInstanceId[0] =
extractDefaultPolymorphicId(field, type);
+
+instancesPerField.put(field, instanceClasses);
}
});
-currentNode.addChildNodes(children);
+// Polymorphic fields
+for (Entry>> e : instancesPerField.entrySet()) {
+Field field = e.getKey();
+Set> instanceClasses = e.getValue();
+Map polymorphicInstances = new HashMap<>();
+
+Set baseClassFields =
Arrays.stream(field.getType().getDeclaredFields())
+.map(Field::getName)
+.collect(Collectors.toSet());
+
+// Collect nodes that correspond to polymorphic instances
+for (Class instanceClass : instanceClasses) {
+ConfigNode instanceTypeNode = createNodeForField(currentNode,
field, instanceClass);
+
polymorphicInstances.put(instanceTypeNode.polymorphicInstanceId(),
instanceTypeNode);
+
+// Each polymorphic instance includes fields from the base
class
+scan(instanceTypeNode, field.getType(), context);
+// And its own fields ignoring base class fields.
+scan(instanceTypeNode, instanceClass, context,
baseClassFields);
Review Comment:
I don't think it's allowed right now, but relying on a hierarchy of Java
classes instead of a hierarchy of "logical" configuration schema representation
might not be a good idea, IF I understood your point correctly
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6325:
URL: https://github.com/apache/ignite-3/pull/6325#discussion_r2237092816
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeScanner.java:
##
@@ -83,33 +81,75 @@ public class ConfigurationTreeScanner {
* @param context The context containing dependency information.
*/
public static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context) {
+scan(currentNode, schemaClass, context, Set.of());
+}
+
+private static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context, Set skipFields) {
assert schemaClass != null &&
schemaClass.getName().startsWith("org.apache.ignite");
Collection> extensions = context.getExtensions(schemaClass);
if (!extensions.isEmpty()) {
extensions.stream()
.sorted(Comparator.comparing(Class::getName)) // Sort for
test stability.
-.forEach(ext -> scan(currentNode, ext, context));
+.forEach(ext -> scan(currentNode, ext, context,
skipFields));
return;
}
-List children = new ArrayList<>();
+Map>> instancesPerField = new HashMap<>();
+String[] defaultInstanceId = new String[1];
+
+// Non-polymorphic fields
configurationClasses(schemaClass).stream()
.flatMap(c -> Arrays.stream(c.getDeclaredFields()))
.filter(field -> !Modifier.isStatic(field.getModifiers()))
+.filter(field -> !skipFields.contains(field.getName()))
.sorted(Comparator.comparing(Field::getName)) // Sort for test
stability.
.forEach(field -> {
-ConfigNode node = createNodeForField(currentNode, field);
+Class type = field.getType();
+Set> instanceClasses =
context.getPolymorphicInstances(type);
+
+// Field itself
+ConfigNode node = createNodeForField(currentNode, field,
type);
+
+if (instanceClasses.isEmpty()) {
+// Single node
+if (!node.isValue()) {
+scan(node, type, context, skipFields);
+}
-children.add(node);
-if (!node.isValue()) {
-scan(node, field.getType(), context);
+currentNode.addChildNodes(List.of(node));
+} else {
+defaultInstanceId[0] =
extractDefaultPolymorphicId(field, type);
+
+instancesPerField.put(field, instanceClasses);
}
});
-currentNode.addChildNodes(children);
+// Polymorphic fields
+for (Entry>> e : instancesPerField.entrySet()) {
+Field field = e.getKey();
+Set> instanceClasses = e.getValue();
+Map polymorphicInstances = new HashMap<>();
+
+Set baseClassFields =
Arrays.stream(field.getType().getDeclaredFields())
+.map(Field::getName)
+.collect(Collectors.toSet());
+
+// Collect nodes that correspond to polymorphic instances
+for (Class instanceClass : instanceClasses) {
+ConfigNode instanceTypeNode = createNodeForField(currentNode,
field, instanceClass);
+
polymorphicInstances.put(instanceTypeNode.polymorphicInstanceId(),
instanceTypeNode);
+
+// Each polymorphic instance includes fields from the base
class
+scan(instanceTypeNode, field.getType(), context);
+// And its own fields ignoring base class fields.
+scan(instanceTypeNode, instanceClass, context,
baseClassFields);
Review Comment:
Should we scan all PolymorphicInstance class parents instead?
E.g. there can be intermediate classes in hierarchy between
`field.getType()` and `instanceClass`.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6325:
URL: https://github.com/apache/ignite-3/pull/6325#discussion_r2237094832
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeScanner.java:
##
@@ -83,33 +81,75 @@ public class ConfigurationTreeScanner {
* @param context The context containing dependency information.
*/
public static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context) {
+scan(currentNode, schemaClass, context, Set.of());
+}
+
+private static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context, Set skipFields) {
assert schemaClass != null &&
schemaClass.getName().startsWith("org.apache.ignite");
Collection> extensions = context.getExtensions(schemaClass);
if (!extensions.isEmpty()) {
extensions.stream()
.sorted(Comparator.comparing(Class::getName)) // Sort for
test stability.
-.forEach(ext -> scan(currentNode, ext, context));
+.forEach(ext -> scan(currentNode, ext, context,
skipFields));
return;
}
-List children = new ArrayList<>();
+Map>> instancesPerField = new HashMap<>();
+String[] defaultInstanceId = new String[1];
+
+// Non-polymorphic fields
configurationClasses(schemaClass).stream()
.flatMap(c -> Arrays.stream(c.getDeclaredFields()))
.filter(field -> !Modifier.isStatic(field.getModifiers()))
+.filter(field -> !skipFields.contains(field.getName()))
.sorted(Comparator.comparing(Field::getName)) // Sort for test
stability.
.forEach(field -> {
-ConfigNode node = createNodeForField(currentNode, field);
+Class type = field.getType();
+Set> instanceClasses =
context.getPolymorphicInstances(type);
+
+// Field itself
+ConfigNode node = createNodeForField(currentNode, field,
type);
+
+if (instanceClasses.isEmpty()) {
+// Single node
+if (!node.isValue()) {
+scan(node, type, context, skipFields);
+}
-children.add(node);
-if (!node.isValue()) {
-scan(node, field.getType(), context);
+currentNode.addChildNodes(List.of(node));
+} else {
+defaultInstanceId[0] =
extractDefaultPolymorphicId(field, type);
+
+instancesPerField.put(field, instanceClasses);
}
});
-currentNode.addChildNodes(children);
+// Polymorphic fields
+for (Entry>> e : instancesPerField.entrySet()) {
+Field field = e.getKey();
+Set> instanceClasses = e.getValue();
+Map polymorphicInstances = new HashMap<>();
+
+Set baseClassFields =
Arrays.stream(field.getType().getDeclaredFields())
+.map(Field::getName)
+.collect(Collectors.toSet());
+
+// Collect nodes that correspond to polymorphic instances
+for (Class instanceClass : instanceClasses) {
+ConfigNode instanceTypeNode = createNodeForField(currentNode,
field, instanceClass);
+
polymorphicInstances.put(instanceTypeNode.polymorphicInstanceId(),
instanceTypeNode);
+
+// Each polymorphic instance includes fields from the base
class
+scan(instanceTypeNode, field.getType(), context);
+// And its own fields ignoring base class fields.
+scan(instanceTypeNode, instanceClass, context,
baseClassFields);
Review Comment:
@ibessonov WDYT?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6325:
URL: https://github.com/apache/ignite-3/pull/6325#discussion_r2237092816
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeScanner.java:
##
@@ -83,33 +81,75 @@ public class ConfigurationTreeScanner {
* @param context The context containing dependency information.
*/
public static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context) {
+scan(currentNode, schemaClass, context, Set.of());
+}
+
+private static void scan(ConfigNode currentNode, Class schemaClass,
ScanContext context, Set skipFields) {
assert schemaClass != null &&
schemaClass.getName().startsWith("org.apache.ignite");
Collection> extensions = context.getExtensions(schemaClass);
if (!extensions.isEmpty()) {
extensions.stream()
.sorted(Comparator.comparing(Class::getName)) // Sort for
test stability.
-.forEach(ext -> scan(currentNode, ext, context));
+.forEach(ext -> scan(currentNode, ext, context,
skipFields));
return;
}
-List children = new ArrayList<>();
+Map>> instancesPerField = new HashMap<>();
+String[] defaultInstanceId = new String[1];
+
+// Non-polymorphic fields
configurationClasses(schemaClass).stream()
.flatMap(c -> Arrays.stream(c.getDeclaredFields()))
.filter(field -> !Modifier.isStatic(field.getModifiers()))
+.filter(field -> !skipFields.contains(field.getName()))
.sorted(Comparator.comparing(Field::getName)) // Sort for test
stability.
.forEach(field -> {
-ConfigNode node = createNodeForField(currentNode, field);
+Class type = field.getType();
+Set> instanceClasses =
context.getPolymorphicInstances(type);
+
+// Field itself
+ConfigNode node = createNodeForField(currentNode, field,
type);
+
+if (instanceClasses.isEmpty()) {
+// Single node
+if (!node.isValue()) {
+scan(node, type, context, skipFields);
+}
-children.add(node);
-if (!node.isValue()) {
-scan(node, field.getType(), context);
+currentNode.addChildNodes(List.of(node));
+} else {
+defaultInstanceId[0] =
extractDefaultPolymorphicId(field, type);
+
+instancesPerField.put(field, instanceClasses);
}
});
-currentNode.addChildNodes(children);
+// Polymorphic fields
+for (Entry>> e : instancesPerField.entrySet()) {
+Field field = e.getKey();
+Set> instanceClasses = e.getValue();
+Map polymorphicInstances = new HashMap<>();
+
+Set baseClassFields =
Arrays.stream(field.getType().getDeclaredFields())
+.map(Field::getName)
+.collect(Collectors.toSet());
+
+// Collect nodes that correspond to polymorphic instances
+for (Class instanceClass : instanceClasses) {
+ConfigNode instanceTypeNode = createNodeForField(currentNode,
field, instanceClass);
+
polymorphicInstances.put(instanceTypeNode.polymorphicInstanceId(),
instanceTypeNode);
+
+// Each polymorphic instance includes fields from the base
class
+scan(instanceTypeNode, field.getType(), context);
+// And its own fields ignoring base class fields.
+scan(instanceTypeNode, instanceClass, context,
baseClassFields);
Review Comment:
Should we scan all PolymorphicInstance class parents instead?
E.g. there can be intermediate classes between field.getType and
instanceClass.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
ibessonov commented on code in PR #6325:
URL: https://github.com/apache/ignite-3/pull/6325#discussion_r2236168936
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -227,15 +254,73 @@ private static boolean rootsMatch(ConfigNode candidate,
ConfigNode current) {
return nameMatches && kindMatches;
}
-private static void validate(Node candidate, Node current,
ComparisonContext context) {
-compareNodes(candidate.node(), current.node(), context);
+private static void validate(ConfigNode candidate, ConfigNode current,
ComparisonContext context) {
+validate(null, candidate, current, context);
}
-private static void validate(ConfigNode candidate, ConfigNode current,
ComparisonContext context) {
+private static void validate(@Nullable String instanceType, ConfigNode
candidate, ConfigNode current, ComparisonContext context) {
+if (!context.errors.isEmpty()) {
+return;
+}
+context.enterInstance(instanceType);
+try {
+compareNodes(candidate, current, context);
+} finally {
+context.leaveInstance();
+}
+}
+
+private static void validate(Node candidate, Node current,
ComparisonContext context) {
if (!context.errors.isEmpty()) {
return;
}
-compareNodes(candidate, current, context);
+
+if (candidate.isPolymorphic() && current.isPolymorphic()) {
+if (!Objects.equals(candidate.defaultPolymorphicInstanceId(),
current.defaultPolymorphicInstanceId())) {
+ConfigNode anyNode =
candidate.nodes().values().iterator().next();
+String message = format(
+"Default polymorphic instance id does not match.
Expected: {} got {}",
Review Comment:
Is this a mistake? I don't think so, we may want to switch the default
implementation of a failure handler, and it's supposed to be a valid change
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka closed pull request #6276: IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot URL: https://github.com/apache/ignite-3/pull/6276 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
zstan commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2219244051
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##
@@ -838,6 +841,76 @@ public void
polymorphicConfigMoveFieldToBase2Subclasses(boolean hasDefault) {
assertIncompatible(root2, root1);
}
}
+
+private static Stream defaultDefault() {
Review Comment:
I think you can use notation like below instead
```
@CartesianTest
void serializationAndDeserialization(
@Values(booleans = {false, true}) boolean force,
@Values(booleans = {false, true}) boolean fromReset
) {
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
ibessonov commented on PR #6276: URL: https://github.com/apache/ignite-3/pull/6276#issuecomment-3096523728 > Fixes an issue with renaming non-leaf nodes because renaming non-leafs nodes is also possible. How large it this fix? Why didn't you do it in a separate Jira? This PR is already big enough, there's no need in artificially inflating it. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218914380
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
+}
+
+private static void validateNewConfigNode(
+@Nullable String instanceType,
+ConfigNode updated,
+ComparisonContext compContext,
+Errors errors
+) {
+if (compContext.shouldIgnore(updated.path())) {
+return;
+}
+errors.push(instanceType);
+
+for (Entry e : updated.children().entrySet()) {
+Node childNode = e.getValue();
+if (childNode.isValue() && !childNode.hasDefault()) {
+errors.addChildError(updated, e.getKey(), "Added a node with
no default value");
+}
+
+if (childNode.isPolymorphic()) {
+for (Map.Entry p :
childNode.nodes().entrySet()) {
+ConfigNode node = p.getValue();
+validateNewConfigNode(p.getKey(), node, compContext,
errors);
+}
+} else {
+ConfigNode node = childNode.node();
+validateNewConfigNode(null, node, compContext, errors);
+}
+}
+
+errors.pop();
+}
+
+private static void doValidateConfigNode(ConfigNode original, ConfigNode
updated, ComparisonContext context, Errors errors) {
+if (context.shouldIgnore(original.path())) {
+return;
+}
+
+if (!match(original, updated)) {
+errors.addError(original, "Node does not match. Previous: " +
original + ". Current: " + updated);
+return;
+}
+
+validateAnnotations(original, updated, errors);
+
+Set originalChildrenNames = original.children().keySet();
+Set updatedChildrenNames = updated.children().keySet();
+
+// Check for removed nodes
+for (String childName : originalChildrenNames) {
+if (updatedChildrenNames.contains(childName)) {
+continue;
+}
+
+String path = original.path() + "." + childName;
+if (context.shouldIgnore(path)) {
+continue;
+}
+
+if (updatedChildrenNames.stream().noneMatch(name -> {
+Node node = updated.children().get(name);
+return node != null &&
node.legacyPropertyNames().contains(childName);
+})) {
+if (context.ignoreAddOrRemove(original, childName)) {
+continue;
+}
+errors.addChildError(original, childName, "Node was removed");
+}
+}
+
+validateChildren(original, updated, context, errors);
+}
+
+private static void validateChildren(
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+for (Entry e : original.children().entrySet()) {
+String childName = e.getKey();
+// Removed noded have already been handled
+if (!updated.children().containsKey(childName)) {
+continue;
+}
+
+Node originalChild = e.getValue();
+Node updatedChild = updated.children().get(childName);
+
+validateChildNode(original, childName, originalChild,
updatedChild, context, errors);
+}
+
+Set originalChildrenNames = original.children().keySet();
+
+// Validate new nodes
+for (Entry e : updated.children().entrySet()) {
+String childName = e.getKey();
+
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218791835
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
+}
+
+private static void validateNewConfigNode(
+@Nullable String instanceType,
+ConfigNode updated,
+ComparisonContext compContext,
+Errors errors
+) {
+if (compContext.shouldIgnore(updated.path())) {
+return;
+}
+errors.push(instanceType);
+
+for (Entry e : updated.children().entrySet()) {
+Node childNode = e.getValue();
+if (childNode.isValue() && !childNode.hasDefault()) {
+errors.addChildError(updated, e.getKey(), "Added a node with
no default value");
+}
+
+if (childNode.isPolymorphic()) {
+for (Map.Entry p :
childNode.nodes().entrySet()) {
+ConfigNode node = p.getValue();
+validateNewConfigNode(p.getKey(), node, compContext,
errors);
+}
+} else {
+ConfigNode node = childNode.node();
+validateNewConfigNode(null, node, compContext, errors);
+}
+}
Review Comment:
I'm not insist, but Visitor will be easier to understand.
Can we add a visitor that call the Consumer for every ConfigNode instance?
The same code could be reused for validating removed values.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218780044
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
+}
+
+private static void validateNewConfigNode(
+@Nullable String instanceType,
+ConfigNode updated,
+ComparisonContext compContext,
+Errors errors
+) {
+if (compContext.shouldIgnore(updated.path())) {
+return;
+}
Review Comment:
Why do we check new node against `deleted regexp`?
New nodes should never be considered as deleted, because (depending on
Configuration module logic)
either we ignore breaking changes or allow values that can't be used, which
looks like a bug.
Should we report a error instead?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218765200
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
Review Comment:
I'd split logic for tracking the current path and error handling.
Use stack structure for path using push\pop operations.
And use separate list for errors.
Maybe both could be incapsulated into ComparisonContext.
```
context.enter(nodeName);
...
context.reportError(error);
context.leave();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218765200
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
Review Comment:
I'd split logic for tracking the current path and error handling.
Use stack structure for path using push\pop operations.
And use separate list for errors.
Maybe both could be incapsulated into ComparisonContext.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
zstan commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218721299
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##
@@ -410,7 +448,7 @@ void testDeleteAfterRename() {
EnumSet.of(Flags.IS_ROOT));
ConfigNode node1Ver1 = new ConfigNode(root, Map.of(Attributes.NAME,
"oldTestCount"), List.of(),
-EnumSet.of(Flags.IS_VALUE));
+EnumSet.of(Flags.IS_VALUE, Flags.HAS_DEFAULT));
Review Comment:
seems this test need to be extended
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
zstan commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218307795
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -185,11 +193,17 @@ void addChildNodes(ConfigNode... childNodes) {
}
/**
- * Returns {@code true} if this node is a root node, {@code false}
otherwise.
+ * Add a polymorhic child node to this node.
Review Comment:
```suggestion
* Add a polymorphic child nodes to this node.
```
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##
@@ -410,7 +448,7 @@ void testDeleteAfterRename() {
EnumSet.of(Flags.IS_ROOT));
ConfigNode node1Ver1 = new ConfigNode(root, Map.of(Attributes.NAME,
"oldTestCount"), List.of(),
-EnumSet.of(Flags.IS_VALUE));
+EnumSet.of(Flags.IS_VALUE, Flags.HAS_DEFAULT));
Review Comment:
is it really need for ("oldTestCount" and "newTestCount") to have
Flags.HAS_DEFAULT ?
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -45,153 +55,141 @@ public static void ensureCompatible(
List actualTrees,
ComparisonContext compContext
) {
-LeafNodesVisitor shuttle = new LeafNodesVisitor(new
Validator(actualTrees), compContext);
-
-for (ConfigNode tree : snapshotTrees) {
-tree.accept(shuttle);
+// Ensure that both collections include the same kinds
+Map> snapshotByKind = new HashMap<>();
+for (ConfigNode node : snapshotTrees) {
+snapshotByKind.computeIfAbsent(node.kind(), (k) -> new
ArrayList<>()).add(node);
}
-}
-/**
- * Compares the configuration trees are equals by dumping their state to
string.
- */
-public static void compare(List tree1, List tree2)
{
-String dump1 = dumpTree(tree1);
-String dump2 = dumpTree(tree2);
-assertEquals(dump1, dump2, "Configuration metadata mismatch");
-}
-
-/**
- * Traverses the tree and triggers validation for leaf nodes.
- */
-private static class LeafNodesVisitor implements ConfigShuttle {
-private final Consumer validator;
-private final ComparisonContext compContext;
-
-private LeafNodesVisitor(Consumer validator,
ComparisonContext compContext) {
-this.validator = validator;
-this.compContext = compContext;
+Map> actualByKind = new HashMap<>();
+for (ConfigNode node : actualTrees) {
+actualByKind.computeIfAbsent(node.kind(), (k) -> new
ArrayList<>()).add(node);
}
-@Override
-public void visit(ConfigNode node) {
-assert node.isRoot() || node.isInnerNode() || node.isNamedNode()
|| node.isValue();
-
-if (node.isValue() && !compContext.shouldIgnore(node.path())) {
-validator.accept(node);
-}
+if (!snapshotByKind.keySet().equals(actualByKind.keySet())) {
+String error = format(
+"Configuration kind does not match. Expected {} but got
{}",
+snapshotByKind.keySet(),
+actualByKind.keySet()
+);
+throw new IllegalStateException(error);
Review Comment:
as i can see - this case is not covered by tests, if so - it need to be
covered.
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +230,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
Review Comment:
let`s use the same namings, as for example in "ensureCompatible" ? i.e.
snapshot and actual ? otherwise it confusing
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compa
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218362830
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -350,5 +387,110 @@ static class Attributes {
static String NAME = "name";
static String KIND = "kind";
static String CLASS = "class";
+static String INSTANCE_TYPE = "instanceType";
+}
+
+/**
+ * Node holds a references either to a single node or to a map of possible
polymorphic nodes with extra `base` node that
+ * includes fields common to all polymorphic instances. Each polymorphic
node has child nodes that represent its fields
+ * and fields common to its polymorphic configuration as well.
+ */
+public static class Node {
+/**
+ * Instance type for a node that represents a `base` class of a
polymorphic node.
+ * A base node includes fields common to all polymorphic instances.
+ */
+static final String BASE_INSTANCE_TYPE = "";
+
+/**
+ * Non-polymorphic node.
+ */
+@JsonProperty
+private ConfigNode single;
+
+/**
+ * All polymorphic instances.
+ */
+@JsonProperty
+private Map polymorphicNodes = new
LinkedHashMap<>();
Review Comment:
Thanks. Fixed.
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -350,5 +387,110 @@ static class Attributes {
static String NAME = "name";
static String KIND = "kind";
static String CLASS = "class";
+static String INSTANCE_TYPE = "instanceType";
+}
+
+/**
+ * Node holds a references either to a single node or to a map of possible
polymorphic nodes with extra `base` node that
+ * includes fields common to all polymorphic instances. Each polymorphic
node has child nodes that represent its fields
+ * and fields common to its polymorphic configuration as well.
+ */
+public static class Node {
+/**
+ * Instance type for a node that represents a `base` class of a
polymorphic node.
+ * A base node includes fields common to all polymorphic instances.
+ */
+static final String BASE_INSTANCE_TYPE = "";
+
+/**
+ * Non-polymorphic node.
+ */
+@JsonProperty
+private ConfigNode single;
Review Comment:
Thanks. Fixed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218362439
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -185,11 +193,17 @@ void addChildNodes(ConfigNode... childNodes) {
}
/**
- * Returns {@code true} if this node is a root node, {@code false}
otherwise.
+ * Add a polymorhic child node to this node.
*/
-@JsonIgnore
-public boolean isRoot() {
-return parent == null;
+void addPolymorhicNode(String name, Map childNodes) {
+boolean nameMatches = childNodes.values().stream().allMatch(n ->
n.name().equals(name));
+if (!nameMatches) {
+throw new IllegalArgumentException("All nodes name should be equal
to " + name);
Review Comment:
Fixed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218362096
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -173,7 +181,7 @@ public Collection childNodes() {
void addChildNodes(Collection childNodes) {
assert !flags.contains(Flags.IS_VALUE) : "Value node can't have
children.";
-childNodes.forEach(e -> childNodeMap.put(e.name(), e));
+childNodes.forEach(e -> childNodeMap.put(e.name(), new Node(e)));
Review Comment:
Fixed.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
zstan commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218276806
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -185,11 +193,17 @@ void addChildNodes(ConfigNode... childNodes) {
}
/**
- * Returns {@code true} if this node is a root node, {@code false}
otherwise.
+ * Add a polymorhic child node to this node.
*/
-@JsonIgnore
-public boolean isRoot() {
-return parent == null;
+void addPolymorhicNode(String name, Map childNodes) {
Review Comment:
```suggestion
void addPolymorphicNode(String name, Map childNodes)
{
```
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -173,7 +181,7 @@ public Collection childNodes() {
void addChildNodes(Collection childNodes) {
assert !flags.contains(Flags.IS_VALUE) : "Value node can't have
children.";
-childNodes.forEach(e -> childNodeMap.put(e.name(), e));
+childNodes.forEach(e -> childNodeMap.put(e.name(), new Node(e)));
Review Comment:
do it need to be checked that childNode is not rewritten ? i.e.
childNodeMap.put return null for each insertion here ?
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -185,11 +193,17 @@ void addChildNodes(ConfigNode... childNodes) {
}
/**
- * Returns {@code true} if this node is a root node, {@code false}
otherwise.
+ * Add a polymorhic child node to this node.
*/
-@JsonIgnore
-public boolean isRoot() {
-return parent == null;
+void addPolymorhicNode(String name, Map childNodes) {
+boolean nameMatches = childNodes.values().stream().allMatch(n ->
n.name().equals(name));
+if (!nameMatches) {
+throw new IllegalArgumentException("All nodes name should be equal
to " + name);
Review Comment:
```suggestion
throw new IllegalArgumentException("All nodes name should be
equal to: " + name);
```
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -350,5 +387,110 @@ static class Attributes {
static String NAME = "name";
static String KIND = "kind";
static String CLASS = "class";
+static String INSTANCE_TYPE = "instanceType";
+}
+
+/**
+ * Node holds a references either to a single node or to a map of possible
polymorphic nodes with extra `base` node that
+ * includes fields common to all polymorphic instances. Each polymorphic
node has child nodes that represent its fields
+ * and fields common to its polymorphic configuration as well.
+ */
+public static class Node {
+/**
+ * Instance type for a node that represents a `base` class of a
polymorphic node.
+ * A base node includes fields common to all polymorphic instances.
+ */
+static final String BASE_INSTANCE_TYPE = "";
+
+/**
+ * Non-polymorphic node.
+ */
+@JsonProperty
+private ConfigNode single;
Review Comment:
```suggestion
private @Nullable ConfigNode single;
```
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##
@@ -350,5 +387,110 @@ static class Attributes {
static String NAME = "name";
static String KIND = "kind";
static String CLASS = "class";
+static String INSTANCE_TYPE = "instanceType";
+}
+
+/**
+ * Node holds a references either to a single node or to a map of possible
polymorphic nodes with extra `base` node that
+ * includes fields common to all polymorphic instances. Each polymorphic
node has child nodes that represent its fields
+ * and fields common to its polymorphic configuration as well.
+ */
+public static class Node {
+/**
+ * Instance type for a node that represents a `base` class of a
polymorphic node.
+ * A base node includes fields common to all polymorphic instances.
+ */
+static final String BASE_INSTANCE_TYPE = "";
+
+/**
+ * Non-polymorphic node.
+ */
+@JsonProperty
+private ConfigNode single;
+
+/**
+ * All polymorphic instances.
+ */
+@JsonProperty
+private Map polymorphicNodes = new
LinkedHashMap<>();
+
+@SuppressWarnings("unused")
+Node() {
+// Default constructor for Jackson deserialization.
+}
+
+Node(ConfigNode single) {
+this.single = single;
+this.polymorphicNodes = null;
+}
+
+Node(Map polymorphicNodes) {
+this.single = null;
+th
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218212560
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +228,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
+}
+
+private static void validateNewConfigNode(
+@Nullable String instanceType,
+ConfigNode updated,
+ComparisonContext compContext,
+Errors errors
+) {
+if (compContext.shouldIgnore(updated.path())) {
+return;
+}
+errors.push(instanceType);
+
+for (Entry e : updated.children().entrySet()) {
+Node childNode = e.getValue();
+if (childNode.isValue() && !childNode.hasDefault()) {
+errors.addChildError(updated, e.getKey(), "Added a node with
no default value");
+}
+
+if (childNode.isPolymorphic()) {
+for (Map.Entry p :
childNode.nodes().entrySet()) {
+ConfigNode node = p.getValue();
+validateNewConfigNode(p.getKey(), node, compContext,
errors);
+}
+} else {
+ConfigNode node = childNode.node();
+validateNewConfigNode(null, node, compContext, errors);
+}
+}
+
+errors.pop();
+}
+
+private static void doValidateConfigNode(ConfigNode original, ConfigNode
updated, ComparisonContext context, Errors errors) {
+if (context.shouldIgnore(original.path())) {
+return;
+}
+
+if (!match(original, updated)) {
+errors.addError(original, "Node does not match. Previous: " +
original + ". Current: " + updated);
+return;
+}
+
+validateAnnotations(original, updated, errors);
+
+Set originalChildrenNames = original.children().keySet();
+Set updatedChildrenNames = updated.children().keySet();
+
+// Check for removed nodes
+for (String childName : originalChildrenNames) {
+if (updatedChildrenNames.contains(childName)) {
+continue;
+}
+
+String path = original.path() + "." + childName;
+if (context.shouldIgnore(path)) {
+continue;
+}
+
+if (updatedChildrenNames.stream().noneMatch(name -> {
+Node node = updated.children().get(name);
+return node != null &&
node.legacyPropertyNames().contains(childName);
+})) {
+if (context.ignoreAddOrRemove(original, childName)) {
+continue;
+}
+errors.addChildError(original, childName, "Node was removed");
+}
+}
+
+validateChildren(original, updated, context, errors);
+}
+
+private static void validateChildren(
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+for (Entry e : original.children().entrySet()) {
+String childName = e.getKey();
+// Removed noded have already been handled
+if (!updated.children().containsKey(childName)) {
+continue;
+}
+
+Node originalChild = e.getValue();
+Node updatedChild = updated.children().get(childName);
+
+validateChildNode(original, childName, originalChild,
updatedChild, context, errors);
+}
+
+Set originalChildrenNames = original.children().keySet();
+
+// Validate new nodes
+for (Entry e : updated.children().entrySet()) {
+String childName = e.getKey();
+
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka commented on code in PR #6276:
URL: https://github.com/apache/ignite-3/pull/6276#discussion_r2218212560
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##
@@ -232,12 +228,444 @@ public static ComparisonContext
create(Set configurationMod
private final KeyIgnorer deletedItems;
+private final Set skipAddRemoveKeys = new HashSet<>();
+
ComparisonContext(Collection deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+boolean shouldIgnore(ConfigNode node, String childName) {
+return shouldIgnore(node.path() + "." + childName);
+}
+
+boolean ignoreAddOrRemove(String path) {
+return skipAddRemoveKeys.contains(path);
+}
+
+boolean ignoreAddOrRemove(ConfigNode node, String childName) {
+return ignoreAddOrRemove(node.path() + "." + childName);
+}
+}
+
+private static void validateConfigNode(
+@Nullable String instanceType,
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+errors.push(instanceType);
+doValidateConfigNode(original, updated, context, errors);
+errors.pop();
+}
+
+private static void validateNewConfigNode(
+@Nullable String instanceType,
+ConfigNode updated,
+ComparisonContext compContext,
+Errors errors
+) {
+if (compContext.shouldIgnore(updated.path())) {
+return;
+}
+errors.push(instanceType);
+
+for (Entry e : updated.children().entrySet()) {
+Node childNode = e.getValue();
+if (childNode.isValue() && !childNode.hasDefault()) {
+errors.addChildError(updated, e.getKey(), "Added a node with
no default value");
+}
+
+if (childNode.isPolymorphic()) {
+for (Map.Entry p :
childNode.nodes().entrySet()) {
+ConfigNode node = p.getValue();
+validateNewConfigNode(p.getKey(), node, compContext,
errors);
+}
+} else {
+ConfigNode node = childNode.node();
+validateNewConfigNode(null, node, compContext, errors);
+}
+}
+
+errors.pop();
+}
+
+private static void doValidateConfigNode(ConfigNode original, ConfigNode
updated, ComparisonContext context, Errors errors) {
+if (context.shouldIgnore(original.path())) {
+return;
+}
+
+if (!match(original, updated)) {
+errors.addError(original, "Node does not match. Previous: " +
original + ". Current: " + updated);
+return;
+}
+
+validateAnnotations(original, updated, errors);
+
+Set originalChildrenNames = original.children().keySet();
+Set updatedChildrenNames = updated.children().keySet();
+
+// Check for removed nodes
+for (String childName : originalChildrenNames) {
+if (updatedChildrenNames.contains(childName)) {
+continue;
+}
+
+String path = original.path() + "." + childName;
+if (context.shouldIgnore(path)) {
+continue;
+}
+
+if (updatedChildrenNames.stream().noneMatch(name -> {
+Node node = updated.children().get(name);
+return node != null &&
node.legacyPropertyNames().contains(childName);
+})) {
+if (context.ignoreAddOrRemove(original, childName)) {
+continue;
+}
+errors.addChildError(original, childName, "Node was removed");
+}
+}
+
+validateChildren(original, updated, context, errors);
+}
+
+private static void validateChildren(
+ConfigNode original,
+ConfigNode updated,
+ComparisonContext context,
+Errors errors
+) {
+for (Entry e : original.children().entrySet()) {
+String childName = e.getKey();
+// Removed noded have already been handled
+if (!updated.children().containsKey(childName)) {
+continue;
+}
+
+Node originalChild = e.getValue();
+Node updatedChild = updated.children().get(childName);
+
+validateChildNode(original, childName, originalChild,
updatedChild, context, errors);
+}
+
+Set originalChildrenNames = original.children().keySet();
+
+// Validate new nodes
+for (Entry e : updated.children().entrySet()) {
+String childName = e.getKey();
+
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
lowka closed pull request #6241: IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot URL: https://github.com/apache/ignite-3/pull/6241 -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6241:
URL: https://github.com/apache/ignite-3/pull/6241#discussion_r2209926940
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/Comp2.java:
##
@@ -0,0 +1,447 @@
+package org.apache.ignite.internal.configuration.compatibility.framework;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Optional;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import
org.apache.ignite.internal.configuration.compatibility.GenerateConfigurationSnapshot;
+import
org.apache.ignite.internal.configuration.compatibility.framework.ConfigNode.NodeReference;
+import
org.apache.ignite.internal.configuration.compatibility.framework.ConfigurationTreeComparator.ComparisonContext;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.internal.util.CollectionUtils;
+
+public class Comp2 {
+
+private static final IgniteLogger LOG =
Loggers.forClass(GenerateConfigurationSnapshot.class);
+
+private final ComparisonContext comparisonContext;
+
+public Comp2() {
+this.comparisonContext = new ComparisonContext();
+}
+
+public Comp2(ComparisonContext comparisonContext) {
+this.comparisonContext = comparisonContext;
+}
+
+public void ensureCompatible(List previousRoots,
List currentRoots) {
+Map> previousRootsByKind =
groupByKind(previousRoots);
+Map> currentRootsByKind =
groupByKind(currentRoots);
+
+// Compare configuration kinds
+if (!previousRootsByKind.keySet().equals(currentRootsByKind.keySet()))
{
+String error = format("Configuration kind do not match. Expected:
{} but got {}",
+previousRootsByKind.keySet(),
+currentRootsByKind.keySet()
+);
+
+throw new IllegalStateException(error);
+}
+
+// Then compare roots one by one
+for (Entry> entry :
previousRootsByKind.entrySet()) {
+Map prev = entry.getValue();
+Map current =
currentRootsByKind.get(entry.getKey());
+
+compareConfigRoots(prev, current);
+}
+}
+
+private void compareConfigRoots(Map previousRoots,
Map currentRoots) {
+Set removed =
CollectionUtils.difference(previousRoots.keySet(), currentRoots.keySet());
+
+// Check config roots
+if (!removed.isEmpty()) {
+String error = format("Incompatible change. Some of the root keys
has been removed.\n"
++ "Removed root keys: {}\n"
++ "Previous root keys: {}\n"
++ "Current root keys: {}\n",
+removed, previousRoots.keySet(), currentRoots.keySet());
+
+throw new IllegalStateException(error);
+}
+
+for (Map.Entry e : previousRoots.entrySet()) {
+ConfigNode current = currentRoots.get(e.getKey());
+if (current == null) {
+// adding a configuration root is a compatible change.
+continue;
+}
+
+ConfigNode previous = e.getValue();
+ensureCompareCompatible(previous, current);
+}
+}
+
+public void ensureCompareCompatible(ConfigNode previous, ConfigNode
current) {
+// List of paths to support polymorphic nodes.
+// Converts configuration trees to a map of paths
+Map> previousNodes = buildPaths(previous);
+Map> currentNodes = buildPaths(current);
+
+if (LOG.isInfoEnabled()) {
+LOG.info("Previous: {}", pathsToString(previousNodes));
+}
+
+if (LOG.isInfoEnabled()) {
+LOG.info("Current: {}", pathsToString(previousNodes));
+}
+
+Set removed =
CollectionUtils.difference(previousNodes.keySet(), currentNodes.keySet());
+Set added = CollectionUtils.difference(currentNodes.keySet(),
previousNodes.keySet());
+Set unchanged =
CollectionUtils.intersect(previousNodes.keySet(), currentNodes.keySet());
+
+List errors = new ArrayList<>();
+
+validateRemovals(removed, comparisonContext, errors);
+validateAdded(currentNodes, added, errors);
+validateRemaining(previousNodes, currentNodes, unchanged, errors);
+
+reportErrors(errors);
+}
+
+private static void validateRemovals(Set keys, ComparisonContext
comparisonContext, List errors) {
+LOG.info("Removed keys: {}", keys);
+
+Set removedFiltered = keys.stream().filter(r ->
!comparisonContext.shouldIgnore(r)).collect(Collectors.toSet());
+if (!remo
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
AMashenkov commented on code in PR #6241:
URL: https://github.com/apache/ignite-3/pull/6241#discussion_r2209903906
##
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/Comp2.java:
##
@@ -0,0 +1,447 @@
+package org.apache.ignite.internal.configuration.compatibility.framework;
+
+import static org.apache.ignite.internal.lang.IgniteStringFormatter.format;
+
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.HashSet;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Optional;
+import java.util.Set;
+import java.util.TreeMap;
+import java.util.TreeSet;
+import java.util.stream.Collectors;
+import
org.apache.ignite.internal.configuration.compatibility.GenerateConfigurationSnapshot;
+import
org.apache.ignite.internal.configuration.compatibility.framework.ConfigNode.NodeReference;
+import
org.apache.ignite.internal.configuration.compatibility.framework.ConfigurationTreeComparator.ComparisonContext;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
+import org.apache.ignite.internal.util.CollectionUtils;
+
+public class Comp2 {
+
+private static final IgniteLogger LOG =
Loggers.forClass(GenerateConfigurationSnapshot.class);
+
+private final ComparisonContext comparisonContext;
+
+public Comp2() {
+this.comparisonContext = new ComparisonContext();
+}
+
+public Comp2(ComparisonContext comparisonContext) {
+this.comparisonContext = comparisonContext;
+}
+
+public void ensureCompatible(List previousRoots,
List currentRoots) {
+Map> previousRootsByKind =
groupByKind(previousRoots);
+Map> currentRootsByKind =
groupByKind(currentRoots);
+
+// Compare configuration kinds
+if (!previousRootsByKind.keySet().equals(currentRootsByKind.keySet()))
{
+String error = format("Configuration kind do not match. Expected:
{} but got {}",
+previousRootsByKind.keySet(),
+currentRootsByKind.keySet()
+);
+
+throw new IllegalStateException(error);
+}
+
+// Then compare roots one by one
+for (Entry> entry :
previousRootsByKind.entrySet()) {
+Map prev = entry.getValue();
+Map current =
currentRootsByKind.get(entry.getKey());
+
+compareConfigRoots(prev, current);
+}
+}
+
+private void compareConfigRoots(Map previousRoots,
Map currentRoots) {
+Set removed =
CollectionUtils.difference(previousRoots.keySet(), currentRoots.keySet());
+
+// Check config roots
+if (!removed.isEmpty()) {
+String error = format("Incompatible change. Some of the root keys
has been removed.\n"
++ "Removed root keys: {}\n"
++ "Previous root keys: {}\n"
++ "Current root keys: {}\n",
+removed, previousRoots.keySet(), currentRoots.keySet());
+
+throw new IllegalStateException(error);
+}
+
+for (Map.Entry e : previousRoots.entrySet()) {
+ConfigNode current = currentRoots.get(e.getKey());
+if (current == null) {
+// adding a configuration root is a compatible change.
+continue;
+}
Review Comment:
```suggestion
```
We get into this branch when previousRoot wasn't found among current ones
(actually, it was removed, not added).
However, we already check the case few lines above.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] IGNITE-25572 Configuration compatibility. Support Polymorphic configuration snapshot [ignite-3]
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 childNodes() {
/**
* Add the child nodes to this node.
*/
-void addChildNodes(Collection childNodes) {
+void addChildReferences(Collection 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.
- * config ver1 has property : prop1
- * config ver2 has renamed property : prop1 -> prop2
- * config ver3 has deleted property : prop1, prop2
+ * Test scenario. config ver1 has property : prop1 config ver2
has renamed property : prop1 -> prop2 config ver3 has
+ * deleted property : prop1, prop2
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 children = new ArrayList<>();
+List 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> instanceClasses =
context.getPolymorphicInstances(field.getType());
+List 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> 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 stabili
