ibessonov commented on code in PR #5816:
URL: https://github.com/apache/ignite-3/pull/5816#discussion_r2102097085
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/Data.java:
##########
@@ -44,10 +45,10 @@ public Data(Map<String, ? extends Serializable> values,
long changeId) {
/**
* Get values.
*
- * @return Values.
+ * @return Unmodifiable map with data values.
*/
public Map<String, ? extends Serializable> values() {
- return values;
+ return Collections.unmodifiableMap(values);
Review Comment:
Field itself should already be unmodifiable, please do that in constructor
instead
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/KeysTrackingConfigurationVisitor.java:
##########
@@ -183,5 +195,32 @@ private void endVisit(int previousKeyLength) {
currentKey.setLength(previousKeyLength);
currentPath.remove(currentPath.size() - 1);
+ legacyNames.remove(legacyNames.size() - 1);
+ }
+
+ /** Calls consumer for all legacy paths. */
+ protected void processLegacyPaths(ArrayList<String> path,
BiConsumer<String, String> legacyKeyNewKeyConsumer) {
Review Comment:
```suggestion
protected void processLegacyPaths(List<String> path, BiConsumer<String,
String> legacyKeyNewKeyConsumer) {
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/KeysTrackingConfigurationVisitor.java:
##########
@@ -183,5 +195,32 @@ private void endVisit(int previousKeyLength) {
currentKey.setLength(previousKeyLength);
currentPath.remove(currentPath.size() - 1);
+ legacyNames.remove(legacyNames.size() - 1);
+ }
+
+ /** Calls consumer for all legacy paths. */
+ protected void processLegacyPaths(ArrayList<String> path,
BiConsumer<String, String> legacyKeyNewKeyConsumer) {
+ if (path.size() == currentPath().size() - 1) {
+ for (String legacyName : legacyNames.get(currentPath().size() -
1)) {
+ String legacyKey = join(appendKey(path, legacyName));
+ String newKey = currentKey();
+
+ legacyKeyNewKeyConsumer.accept(legacyKey, newKey);
+ }
+
+ path.remove(path.size() - 1);
Review Comment:
Please explain this part. This method didn't add anything to the list in the
code above, why does it remove something?
This code should be refactored or properly documented, this is too confusing
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationFlattener.java:
##########
@@ -126,6 +139,12 @@ public Void doVisitLeafNode(Field field, String key,
Serializable newVal) {
resMap.put(currentKey(), deletion ? null : newVal);
}
+ processLegacyPaths(new ArrayList<>(), (legacyKey, newKey) -> {
Review Comment:
What about non-leaf nodes? You only call it in `doVisitLeafNode`
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/KeysTrackingConfigurationVisitor.java:
##########
@@ -183,5 +195,32 @@ private void endVisit(int previousKeyLength) {
currentKey.setLength(previousKeyLength);
currentPath.remove(currentPath.size() - 1);
+ legacyNames.remove(legacyNames.size() - 1);
+ }
+
+ /** Calls consumer for all legacy paths. */
Review Comment:
Please expand this description
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -754,6 +761,21 @@ private static Data mergeData(Data currentData, Data
delta) {
return new Data(newState, delta.changeId());
}
+ private static void ignoreLegacyKeys(StorageRoots oldStorageRoots,
Map<String, ? extends Serializable> changedValues) {
+ oldStorageRoots.roots.traverseChildren(new
KeysTrackingConfigurationVisitor<>() {
+ @Override
+ protected Object doVisitLeafNode(Field field, String key,
Serializable val) {
+ var path = new ArrayList<String>();
Review Comment:
Could be inlined
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/KeysTrackingConfigurationVisitor.java:
##########
@@ -183,5 +195,32 @@ private void endVisit(int previousKeyLength) {
currentKey.setLength(previousKeyLength);
currentPath.remove(currentPath.size() - 1);
+ legacyNames.remove(legacyNames.size() - 1);
+ }
+
+ /** Calls consumer for all legacy paths. */
+ protected void processLegacyPaths(ArrayList<String> path,
BiConsumer<String, String> legacyKeyNewKeyConsumer) {
+ if (path.size() == currentPath().size() - 1) {
+ for (String legacyName : legacyNames.get(currentPath().size() -
1)) {
+ String legacyKey = join(appendKey(path, legacyName));
+ String newKey = currentKey();
+
+ legacyKeyNewKeyConsumer.accept(legacyKey, newKey);
+ }
+
+ path.remove(path.size() - 1);
+
+ return;
+ }
+
+ for (String legacyName : legacyNames.get(path.size())) {
+ path.add(legacyName);
+
+ processLegacyPaths(path, legacyKeyNewKeyConsumer);
+ }
+
+ path.add(currentPath().get(path.size()));
+
+ processLegacyPaths(path, legacyKeyNewKeyConsumer);
Review Comment:
I'm not convinced that this is a right choice, time complexity of visiting
the configuration will be too high. Why does this code exist, is it even tested?
The way I see it - there should be two methods, one that does it, and one
that does not. And the latter should be private. This code requires some very
careful consideration
--
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
