You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2021/08/03 12:14:35 UTC
[GitHub] [ignite-3] sashapolo commented on a change in pull request #244: IGNITE-15166 Implemented "rename" API for named lists configuration.
sashapolo commented on a change in pull request #244:
URL: https://github.com/apache/ignite-3/pull/244#discussion_r681675664
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -137,9 +142,9 @@ public void childNode() throws Exception {
assertEquals(List.of("parent", "child", "str"), log);
}
- /** */
+ /** Tests notifications validity when new named list element is created. */
Review comment:
```suggestion
/** Tests notifications validity when a new named list element is created. */
```
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
##########
@@ -305,4 +308,20 @@ public void defaultsOnInit() throws Exception {
assertEquals("bar", root.childsList().get("name").defStr());
}
+ /**
+ * Convert root node into patching configuration source for the super root.
Review comment:
```suggestion
* Converts a root node into a patching configuration source for the super root.
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationUtil.java
##########
@@ -37,6 +36,9 @@
/** */
public class ConfigurationUtil {
+ /** Configuration that does nothing but copy of the configuration value. */
Review comment:
nothing but + noun means "a lot of", which is not applicable here. Maybe something simple, like `Configuration source that copies values`, is better.
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -137,9 +142,9 @@ public void childNode() throws Exception {
assertEquals(List.of("parent", "child", "str"), log);
}
- /** */
+ /** Tests notifications validity when new named list element is created. */
Review comment:
same applies to all other javadocs in this class
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
##########
@@ -305,4 +308,20 @@ public void defaultsOnInit() throws Exception {
assertEquals("bar", root.childsList().get("name").defStr());
}
+ /**
+ * Convert root node into patching configuration source for the super root.
+ *
+ * @param rootKey Root key.
+ * @param changes Root node.
Review comment:
`changes` is a strange name for the root node...
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/util/ConfigurationUtilTest.java
##########
@@ -423,4 +432,21 @@ public void patch() {
private ParentChange copy(ParentView parent) {
return (ParentChange)((ConstructableTreeNode)parent).copy();
}
+
+ /**
+ * Apply changes on top of existing node. Creates completely new object while reusing parts of the original tree
Review comment:
```suggestion
* Applies changes on top of an existing node. Creates a completely new object while reusing parts of the original tree
```
##########
File path: modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -82,7 +84,25 @@
NamedListChange<Change> createOrUpdate(String key, Consumer<Change> valConsumer);
/**
- * Remove the value from named list configuration.
+ * Renames the existing value in the named list configuration. Element with key {@code oldKey} must exist and key
+ * {@code newKey} must not. Error will occur if {@code newKey} has just been deleted on the same
+ * {@link NamedListChange} instance. It is necessary to simplify distinguisnment between
Review comment:
```suggestion
* {@link NamedListChange} instance (to distinguish between ...
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java
##########
@@ -233,32 +232,34 @@ public void initialize(ConfigurationType storageType) throws ConfigurationValida
assert configurationStorage != null : storageType;
- StorageRoots storageRoots = storagesRootsMap.get(storageType);
-
- SuperRoot superRoot = storageRoots.roots;
- SuperRoot defaultsNode = new SuperRoot(rootCreator());
-
- addDefaults(superRoot, defaultsNode);
+ try {
+ changeInternally(
+ new ConfigurationSource() {
Review comment:
please extract this into a variable
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -222,20 +338,62 @@ public void reorderKeys(List<String> orderedKeys) {
/** {@inheritDoc} */
@Override public void construct(String key, ConfigurationSource src) {
if (src == null)
- map.put(key, null);
- else {
- N val = map.get(key);
-
- val = val == null ? valSupplier.get() : (N)val.copy();
-
- map.put(key, val);
-
- src.descend(val);
- }
+ delete(key);
+ else
+ createOrUpdate(key, src::descend);
}
/** {@inheritDoc} */
@Override public NamedListNode<N> copy() {
return new NamedListNode<>(this);
}
+
+ /**
+ * Descriptor for internal named list element representation. Has node itself and its internal id.
+ *
+ * @param <N> Type of the node.
+ */
+ private static class ElementDescriptor<N extends InnerNode> implements Cloneable {
+ /** Element's internal id. */
+ public String internalId;
+
+ /** Element node value. */
+ public N value;
+
+ /**
+ * Constructor.
+ *
+ * @param value Node instance.
+ */
+ ElementDescriptor(N value) {
+ this.value = value;
+ internalId = UUID.randomUUID().toString().replace("-", "");
+ }
+
+ /**
+ * Private constructor with entire fields list.
+ *
+ * @param internalId Internal id.
+ * @param value Node instance.
+ */
+ private ElementDescriptor(String internalId, N value) {
+ this.internalId = internalId;
+ this.value = value;
+ }
+
+ /**
+ * Makes a copy of the element descriptor. Not to be confused with {@link #clone()}.
+ *
+ * @return New instance with the same internal id but copied node instance.
+ * @see InnerNode#copy()
+ */
+ public ElementDescriptor<N> copy() {
+ return new ElementDescriptor<>(internalId, (N)value.copy());
+ }
+
+ /** {@inheritDoc} */
+ @Override protected ElementDescriptor<N> clone() {
Review comment:
I would suggest using a custom method, like `shallowCopy` instead, `clone` is indeed confusing
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -222,20 +338,62 @@ public void reorderKeys(List<String> orderedKeys) {
/** {@inheritDoc} */
@Override public void construct(String key, ConfigurationSource src) {
if (src == null)
- map.put(key, null);
- else {
- N val = map.get(key);
-
- val = val == null ? valSupplier.get() : (N)val.copy();
-
- map.put(key, val);
-
- src.descend(val);
- }
+ delete(key);
+ else
+ createOrUpdate(key, src::descend);
}
/** {@inheritDoc} */
@Override public NamedListNode<N> copy() {
return new NamedListNode<>(this);
}
+
+ /**
+ * Descriptor for internal named list element representation. Has node itself and its internal id.
+ *
+ * @param <N> Type of the node.
+ */
+ private static class ElementDescriptor<N extends InnerNode> implements Cloneable {
+ /** Element's internal id. */
+ public String internalId;
+
+ /** Element node value. */
+ public N value;
+
+ /**
+ * Constructor.
+ *
+ * @param value Node instance.
+ */
+ ElementDescriptor(N value) {
+ this.value = value;
+ internalId = UUID.randomUUID().toString().replace("-", "");
Review comment:
why do you remove the dashes? I think it should be reflected in a comment
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DynamicConfiguration.java
##########
@@ -100,8 +100,12 @@ protected DynamicConfiguration(
return refreshValue();
}
- /** {@inheritDoc} */
- @Override public Map<String, ConfigurationProperty<?, ?>> members() {
+ /**
+ * Returns all child nodes of current configuration tree node.
+ *
+ * @return Map from {@code String} to a corresponding {@link ConfigurationProperty}.
Review comment:
what does this `String` represent in this context?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/internal/configuration/DynamicConfiguration.java
##########
@@ -100,8 +100,12 @@ protected DynamicConfiguration(
return refreshValue();
}
- /** {@inheritDoc} */
- @Override public Map<String, ConfigurationProperty<?, ?>> members() {
+ /**
+ * Returns all child nodes of current configuration tree node.
Review comment:
```suggestion
* Returns all child nodes of the current configuration tree node.
```
##########
File path: modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java
##########
@@ -305,4 +308,20 @@ public void defaultsOnInit() throws Exception {
assertEquals("bar", root.childsList().get("name").defStr());
}
+ /**
+ * Convert root node into patching configuration source for the super root.
Review comment:
what is a `patching configuration source`?
--
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org