zstan commented on code in PR #6305:
URL: https://github.com/apache/ignite-3/pull/6305#discussion_r2228111458
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##########
@@ -460,12 +561,55 @@ private static ConfigNode createRoot(String name) {
return ConfigNode.createRoot(name, Object.class,
ConfigurationType.LOCAL, true);
}
- private static ConfigNode createChild(ConfigNode root1, String name) {
+ private static ConfigNode createNode(String name, String className) {
+ return createNode(name, className, Set.of(), List.of());
+ }
+
+ private static ConfigNode createNode(String name, String className,
Set<String> legacyNames, List<String> deletedPrefixes) {
return new ConfigNode(
- root1,
+ null,
+ Map.of(Attributes.NAME, name, Attributes.CLASS, className),
+ List.of(),
+ EnumSet.noneOf(Flags.class),
+ legacyNames,
+ deletedPrefixes
+ );
+ }
+
+ private static ConfigNode createChild(ConfigNode root, String name) {
+ return new ConfigNode(
+ root,
Map.of(ConfigNode.Attributes.NAME, name, Attributes.CLASS,
int.class.getCanonicalName()),
List.of(),
- EnumSet.of(Flags.IS_VALUE)
+ EnumSet.of(Flags.IS_VALUE, Flags.HAS_DEFAULT)
+ );
+ }
+
+ private static ConfigNode createChild(String name) {
+ return createChild(name, Set.of(), Set.of(), List.of());
+ }
+
+ private static ConfigNode createChild(String name, Set<Flags> flags) {
+ return createChild(name, flags, Set.of(), List.of());
+ }
+
+ private static ConfigNode createChild(String name, Set<Flags> flags,
Set<String> legacyNames, List<String> deletedPrefixes) {
+ Set<Flags> valueFlags = new HashSet<>();
+ valueFlags.add(Flags.IS_VALUE);
Review Comment:
do we really want to override flags if they non empty ? i think this can
lead to possible erroneous tests in future, if you need such a behavior - i
think method need to be renamed, wdyt ?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigNode.java:
##########
@@ -153,11 +153,18 @@ public String type() {
return attributes.get(Attributes.CLASS);
}
+ /**
+ * Returns flags for this node.
+ */
+ public Set<Flags> flags() {
+ return flags;
+ }
+
/**
* Returns the child nodes of this node.
*/
- public Collection<ConfigNode> childNodes() {
- return childNodeMap.values();
+ public Map<String, Node> children() {
+ return childNodeMap;
Review Comment:
i found that childNodeMap used only values, seems keys are not involved -
thus - can we change it into Set ? or i miss smth ?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparatorSelfTest.java:
##########
@@ -491,12 +635,17 @@ private static void assertIncompatible(List<ConfigNode>
oldConfig, List<ConfigNo
private static void assertIncompatible(List<ConfigNode> oldConfig,
List<ConfigNode> newConfig, ComparisonContext compContext) {
try {
- assertCompatible(oldConfig, newConfig, compContext);
- } catch (IllegalStateException ignore) {
+ ConfigurationTreeComparator.ensureCompatible(oldConfig, newConfig,
compContext);
+ } catch (IllegalStateException e) {
// Expected exception
+ System.err.println("Error: " + e.getMessage());
return;
}
fail("Compatibility check passed unexpectedly.");
}
+
+ private static Set<Flags> createFlags(boolean hasDefault) {
+ return hasDefault ? Set.of(Flags.HAS_DEFAULT, Flags.IS_VALUE) :
Set.of(Flags.IS_VALUE);
Review Comment:
why not EnumSet.of here ? some upper code can be simplified, or i miss smth ?
##########
modules/runner/src/test/java/org/apache/ignite/internal/configuration/compatibility/framework/ConfigurationTreeComparator.java:
##########
@@ -232,12 +149,182 @@ public static ComparisonContext
create(Set<ConfigurationModule> configurationMod
private final KeyIgnorer deletedItems;
+ private final List<String> errors = new ArrayList<>();
+
ComparisonContext(Collection<String> deletedPrefixes) {
this.deletedItems =
KeyIgnorer.fromDeletedPrefixes(deletedPrefixes);
}
boolean shouldIgnore(String path) {
return deletedItems.shouldIgnore(path);
}
+
+ void reset() {
+ errors.clear();
+ }
+
+ void addError(Node node, String error) {
+ reportError(node.path(), error);
+ }
+
+ void addError(ConfigNode node, String error) {
+ reportError(node.path(), error);
+ }
+
+ private void reportError(String path, String error) {
+ String message = format("Node: {}: {}", path, error);
+ errors.add(message);
+ }
+
+ private void throwIfNotEmpty() {
+ if (errors.isEmpty()) {
+ return;
+ }
+
+ StringBuilder message = new StringBuilder("There are incompatible
changes:").append(System.lineSeparator());
+ for (String error : errors) {
+
message.append('\t').append(error).append(System.lineSeparator());
+ }
+
+ throw new IllegalStateException(message.toString());
+ }
+ }
+
+
+ private static void compareRoots(List<ConfigNode> roots1, List<ConfigNode>
roots2, ComparisonContext context) {
+ List<ConfigNode> removed = new ArrayList<>();
+ List<ConfigNode> added = new ArrayList<>();
+ List<ConfigNode> copy2 = new ArrayList<>(roots2);
+
+ for (ConfigNode root1 : roots1) {
+ boolean matchFound = false;
+
+ for (ConfigNode root2 : new ArrayList<>(copy2)) {
+ if (rootsMatch(root1, root2)) {
+ copy2.remove(root2);
+ validate(root1, root2, context);
+ matchFound = true;
+ break;
+ }
+ }
+
+ if (!matchFound) {
+ removed.add(root1);
+ }
+ }
+
+ added.addAll(copy2);
+
+ // Validate new roots
+ validateNew(added, context);
+ // Reject removed roots.
+ validateRemoved(removed, context);
+ }
+
+ private static boolean rootsMatch(ConfigNode a, ConfigNode b) {
+ boolean nameMatches = Objects.equals(a.name(), b.name());
+ boolean kindMatches = Objects.equals(a.kind(), b.kind());
+ return nameMatches && kindMatches;
+ }
+
+ private static void validate(Node a, Node b, ComparisonContext context) {
+ compareNodes(a.node(), b.node(), context);
+ }
+
+ private static void validate(ConfigNode candidate, ConfigNode current,
ComparisonContext context) {
+ if (!context.errors.isEmpty()) {
+ return;
+ }
+ compareNodes(candidate, current, context);
Review Comment:
validate call compareNodes with param namings : compareNodes(ConfigNode a,
ConfigNode b - it confusing, can we use the same namings through such functions
?
--
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
