ibessonov commented on code in PR #5728:
URL: https://github.com/apache/ignite-3/pull/5728#discussion_r2071205802
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -118,6 +124,9 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
/** Flag indicating whether the component is started. */
private final AtomicBoolean started = new AtomicBoolean(false);
+ /** Keys that were deleted, but still present in configuration. */
+ private Collection<String> ignoredKeys;
Review Comment:
Why is this a field? Such a collection might only correspond to a single
specific configuration version. I don't trust it, please either expand the
comment with a better explanation, or remove this field
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -223,6 +233,11 @@ public ConfigurationChanger(
this.configurationValidator = configurationValidator;
this.rootKeys = rootKeys.stream().collect(toMap(RootKey::key,
identity()));
this.migrator = migrator;
+
+ this.deletedPrefixes = deletedPrefixes.stream()
+ .map(deletedKey -> deletedKey.replace(".", "\\.").replace("*",
".*") + ".*")
Review Comment:
Let's assume that I deleted `ignite.foo` and introduced `ignite.fooMillis`
few months later. Will the new configuration match the old prefix for deleted
configuration? It should not match it. Please add such a test
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -223,6 +233,11 @@ public ConfigurationChanger(
this.configurationValidator = configurationValidator;
this.rootKeys = rootKeys.stream().collect(toMap(RootKey::key,
identity()));
this.migrator = migrator;
+
+ this.deletedPrefixes = deletedPrefixes.stream()
+ .map(deletedKey -> deletedKey.replace(".", "\\.").replace("*",
".*") + ".*")
Review Comment:
```suggestion
.map(deletedKey -> deletedKey.replace(".",
"\\.").replace("*", "[^.]*") + ".*")
```
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -136,4 +136,14 @@ default void
patchConfigurationWithDynamicDefaults(SuperRootChange rootChange) {
default void migrateDeprecatedConfigurations(SuperRootChange
superRootChange) {
// No-op.
}
+
+ /**
+ * Returns a collection of prefixes, removed from configuration. Keys that
match any of the prefixes
+ * in this collection will be deleted.
Review Comment:
Please describe the format of prefixes. They might include `*`, right?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -223,6 +233,11 @@ public ConfigurationChanger(
this.configurationValidator = configurationValidator;
this.rootKeys = rootKeys.stream().collect(toMap(RootKey::key,
identity()));
this.migrator = migrator;
+
+ this.deletedPrefixes = deletedPrefixes.stream()
+ .map(deletedKey -> deletedKey.replace(".", "\\.").replace("*",
".*") + ".*")
Review Comment:
Be carefult
--
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
