ibessonov commented on code in PR #5728:
URL: https://github.com/apache/ignite-3/pull/5728#discussion_r2073160730
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/hocon/HoconObjectConfigurationSource.java:
##########
@@ -87,6 +99,12 @@ public void descend(ConstructableTreeNode node) {
}
private void parseConfigEntry(String key, ConfigValue hoconCfgValue,
ConstructableTreeNode node) {
+ for (Pattern deletedPrefix : deletedPrefixes) {
+ if (deletedPrefix.matcher(join(appendKey(path, key))).matches()) {
+ return;
+ }
+ }
Review Comment:
Such a pattern is repeated multiple times. Maybe we should introduce an
interface, this way we'll be able to replace its implementation with a more
efficient one in the future!
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DeletedKeysFilter.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite.internal.configuration;
+
+import java.io.Serializable;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+import java.util.stream.Collectors;
+
+class DeletedKeysFilter {
+ /**
+ * Filters out keys that match the deleted prefixes and returns the
ignored keys.
+ *
+ * @param values The map of values to filter.
+ * @param deletedPrefixes Patterns of prefixes, deleted from the
configuration.
+ * @return A collection of keys that were ignored.
+ */
+ static List<String> ignoreDeleted(
+ Map<String, ? extends Serializable> values,
+ Collection<Pattern> deletedPrefixes
+ ) {
+ if (deletedPrefixes.isEmpty()) {
+ return List.of();
+ }
+
+ List<String> ignoredKeys = values.keySet().stream()
+ .filter(key -> isDeleted(key, deletedPrefixes))
+ .collect(Collectors.toList());
+
+ for (String key : ignoredKeys) {
+ values.remove(key);
+ }
+
+ return ignoredKeys;
Review Comment:
Please rewrite using `values().keySet().removeIf(...)`, code will become
smaller and easier
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -136,4 +138,24 @@ 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.
+ * <p>Use ignite.my.deleted.property for regular deleted properties</p>
Review Comment:
```suggestion
* <p>Use {@code ignite.my.deleted.property} for regular deleted
properties</p>
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/hocon/HoconListConfigurationSource.java:
##########
@@ -105,6 +112,12 @@ public void descend(ConstructableTreeNode node) {
String syntheticKeyName = ((NamedListNode<?>) node).syntheticKeyName();
+ for (Pattern deletedPrefix : deletedPrefixes) {
+ if (deletedPrefix.matcher(join(appendKey(path,
syntheticKeyName))).matches()) {
Review Comment:
Please at least call `join` once before the loop. This code is kind of
inefficient, I don't like it.
I'm not sure we should spend time optimizing it right now, but it just
bothers me, you know
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -136,4 +138,24 @@ 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.
+ * <p>Use ignite.my.deleted.property for regular deleted properties</p>
+ * <p>ignite.list.*.deletedProperty - for named list elements. Arbitrarily
nested named lists are supported</p>
+ *
+ * @return A collection of prefixes of deleted keys.
+ */
+ default Collection<String> deletedPrefixes() {
+ return emptySet();
+ }
+
+ /** Compiles @{link #deletedPrefixes()} into regex patterns. Shouldn't be
overwritten. */
+ default Collection<Pattern> deletedPrefixesPatterns() {
Review Comment:
Can it be a static method? I don't trust this `Shouldn't be overwritten` in
interface
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/ConfigurationModule.java:
##########
@@ -136,4 +138,24 @@ 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.
+ * <p>Use ignite.my.deleted.property for regular deleted properties</p>
+ * <p>ignite.list.*.deletedProperty - for named list elements. Arbitrarily
nested named lists are supported</p>
Review Comment:
```suggestion
* <p>{@code ignite.list.*.deletedProperty} - for named list elements.
Arbitrarily nested named lists are supported</p>
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -225,6 +252,10 @@ private CompletableFuture<Void>
combineFutures(Collection<CompletableFuture<?>>
};
}
+ public Collection<Pattern> deletedPrefixes() {
Review Comment:
I wander if we should use a designated interface instead of
`Collection<Pattern>`. Maybe in the future, I don't want to bother you anymore
--
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
