sashapolo commented on a change in pull request #244:
URL: https://github.com/apache/ignite-3/pull/244#discussion_r678116081
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -138,7 +144,7 @@ public void childNode() throws Exception {
/** */
@Test
public void namedListNode() throws Exception {
Review comment:
please add some comments across this method about what you are actually
testing, it's quite hard to understand. Maybe it would be easier to split this
test into 4 smaller tests
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -246,9 +295,66 @@ else if (ctx.newValue().size() == 0) {
log.clear();
configuration.change(parent ->
- parent.changeElements(elements -> elements.delete("name"))
+ parent.changeElements(elements -> elements.rename("name",
"newName"))
+ ).get(1, SECONDS);
+
+ assertEquals(List.of("parent", "elements", "rename"), log);
+
+ log.clear();
+
+ configuration.change(parent ->
+ parent.changeElements(elements -> elements.delete("newName"))
).get(1, SECONDS);
assertEquals(List.of("parent", "elements", "delete"), log);
}
+
+ /** */
+ @Test
+ @Disabled("Will be fixed in
https://issues.apache.org/jira/browse/IGNITE-15193";)
+ public void dataRace() throws Exception {
+ configuration.change(parent -> parent.changeElements(elements ->
+ elements.create("name", e -> {}))
+ ).get(1, SECONDS);
+
+ CountDownLatch wait = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+
+ List<String> log = new CopyOnWriteArrayList<>();
+
+ configuration.listen(ctx -> {
+ try {
+ wait.await();
Review comment:
it's not a very good practice to use infinite `await`, please consider
using a version with a timeout
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -17,26 +17,37 @@
package org.apache.ignite.internal.configuration.tree;
+import java.io.Serializable;
import java.util.Map;
-import org.apache.ignite.configuration.NamedListChange;
+import java.util.regex.Pattern;
import org.apache.ignite.configuration.annotation.Config;
import org.apache.ignite.configuration.annotation.ConfigurationRoot;
import org.apache.ignite.configuration.annotation.NamedConfigValue;
import org.apache.ignite.configuration.annotation.Value;
import org.apache.ignite.internal.configuration.TestConfigurationChanger;
import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.hasEntry;
+import static org.hamcrest.Matchers.hasToString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.matchesPattern;
import static org.junit.jupiter.api.Assertions.assertThrows;
-/** Test for keys ordering in named list nodes. */
+/** Test for named list nodes. */
public class NamedListOrderTest {
+ /** Named list entry od pattern. */
+ private final Pattern idPattern = Pattern.compile("(\\d|\\w){32}");
Review comment:
`\w` already includes digits
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -82,7 +82,21 @@
NamedListChange<Change> createOrUpdate(String key, Consumer<Change>
valConsumer);
/**
- * Remove the value from named list configuration.
+ * Renames the existing value in the named list configuration.
+ *
+ * @param oldKey Key for the value to be updated.
+ * @param newKey New key for the same value.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If an element with name {@code newKey}
already exists, or an element with name
+ * {@code oldKey} doesn't exits, or {@link #delete(String)} has been
invoked with the either {@code newKey}
+ * or {@code oldKey} previously.
Review comment:
```suggestion
* or the {@code oldKey}.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
+ * not be changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is be performed when
Review comment:
```suggestion
* have not been changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is performed when
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
+ * not be changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is be performed when
Review comment:
The sentence is clearly unfinished
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -80,12 +88,12 @@ private NamedListNode(NamedListNode<N> node) {
/** {@inheritDoc} */
@Override public final N get(String key) {
- return map.get(key);
+ return
Optional.ofNullable(map.get(key)).map(IgniteBiTuple::getValue).orElse(null);
}
/** {@inheritDoc} */
@Override public N get(int index) throws IndexOutOfBoundsException {
- return map.get(index);
+ return
Optional.ofNullable(map.get(index)).map(IgniteBiTuple::getValue).orElse(null);
Review comment:
same here
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -156,18 +164,44 @@ private NamedListNode(NamedListNode<N> node) {
if (map.containsKey(key) && map.get(key) == null)
throw new IllegalArgumentException("You can't create entity that
has just been deleted [key=" + key + ']');
- N val = map.get(key);
+ IgniteBiTuple<String, N> pair = map.get(key);
- if (val == null)
- map.put(key, val = valSupplier.get());
- else
- map.put(key, val = (N)val.copy());
+ pair = pair == null
+ ? new IgniteBiTuple<>(newId(), valSupplier.get())
+ : new IgniteBiTuple<>(pair.getKey(), (N)pair.getValue().copy());
- valConsumer.accept(val);
+ map.put(key, pair);
+
+ valConsumer.accept(pair.getValue());
return this;
}
+ /** {@inheritDoc} */
+ @Override public NamedListChange<N> rename(String oldKey, String newKey) {
+ Objects.requireNonNull(oldKey, "oldKey");
+ Objects.requireNonNull(newKey, "newKey");
+
+ if (!map.containsKey(oldKey))
+ throw new IllegalArgumentException("Element with name " + oldKey +
" does not exist.");
+
+ if (map.get(oldKey) == null) {
+ throw new IllegalArgumentException(
+ "You can't rename entity that has just been deleted [key=" +
oldKey + ']'
Review comment:
```suggestion
"Can't rename entity that has just been deleted [key=" +
oldKey + ']'
```
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -17,26 +17,37 @@
package org.apache.ignite.internal.configuration.tree;
+import java.io.Serializable;
import java.util.Map;
-import org.apache.ignite.configuration.NamedListChange;
+import java.util.regex.Pattern;
import org.apache.ignite.configuration.annotation.Config;
import org.apache.ignite.configuration.annotation.ConfigurationRoot;
import org.apache.ignite.configuration.annotation.NamedConfigValue;
import org.apache.ignite.configuration.annotation.Value;
import org.apache.ignite.internal.configuration.TestConfigurationChanger;
import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.hasEntry;
+import static org.hamcrest.Matchers.hasToString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.matchesPattern;
import static org.junit.jupiter.api.Assertions.assertThrows;
-/** Test for keys ordering in named list nodes. */
+/** Test for named list nodes. */
public class NamedListOrderTest {
+ /** Named list entry od pattern. */
Review comment:
```suggestion
/** Named list entry id pattern. */
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
Review comment:
```suggestion
* {@link #onUpdate(ConfigurationNotificationEvent)} with the difference
that the content of the element might
```
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -246,9 +295,66 @@ else if (ctx.newValue().size() == 0) {
log.clear();
configuration.change(parent ->
- parent.changeElements(elements -> elements.delete("name"))
+ parent.changeElements(elements -> elements.rename("name",
"newName"))
+ ).get(1, SECONDS);
+
+ assertEquals(List.of("parent", "elements", "rename"), log);
+
+ log.clear();
+
+ configuration.change(parent ->
+ parent.changeElements(elements -> elements.delete("newName"))
).get(1, SECONDS);
assertEquals(List.of("parent", "elements", "delete"), log);
}
+
+ /** */
+ @Test
+ @Disabled("Will be fixed in
https://issues.apache.org/jira/browse/IGNITE-15193";)
+ public void dataRace() throws Exception {
+ configuration.change(parent -> parent.changeElements(elements ->
+ elements.create("name", e -> {}))
+ ).get(1, SECONDS);
+
+ CountDownLatch wait = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+
+ List<String> log = new CopyOnWriteArrayList<>();
+
+ configuration.listen(ctx -> {
+ try {
+ wait.await();
+ }
+ catch (InterruptedException e) {
+ fail(e.getMessage());
+ }
+
+ release.countDown();
+
+ return completedFuture(null);
+ });
+
+ configuration.elements().get("name").listen(ctx -> {
+ assertNull(ctx.newValue());
+
+ log.add("deleted");
+
+ return completedFuture(null);
+ });
+
+ Future<Void> fut = configuration.change(parent ->
parent.changeElements(elements ->
+ elements.delete("name"))
+ );
+
+ wait.countDown();
+
+ configuration.elements();
+
+ release.await();
Review comment:
same here
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/notifications/ConfigurationListenerTest.java
##########
@@ -246,9 +295,66 @@ else if (ctx.newValue().size() == 0) {
log.clear();
configuration.change(parent ->
- parent.changeElements(elements -> elements.delete("name"))
+ parent.changeElements(elements -> elements.rename("name",
"newName"))
+ ).get(1, SECONDS);
+
+ assertEquals(List.of("parent", "elements", "rename"), log);
+
+ log.clear();
+
+ configuration.change(parent ->
+ parent.changeElements(elements -> elements.delete("newName"))
).get(1, SECONDS);
assertEquals(List.of("parent", "elements", "delete"), log);
}
+
+ /** */
+ @Test
+ @Disabled("Will be fixed in
https://issues.apache.org/jira/browse/IGNITE-15193";)
+ public void dataRace() throws Exception {
+ configuration.change(parent -> parent.changeElements(elements ->
+ elements.create("name", e -> {}))
+ ).get(1, SECONDS);
+
+ CountDownLatch wait = new CountDownLatch(1);
+ CountDownLatch release = new CountDownLatch(1);
+
+ List<String> log = new CopyOnWriteArrayList<>();
+
+ configuration.listen(ctx -> {
+ try {
+ wait.await();
+ }
+ catch (InterruptedException e) {
+ fail(e.getMessage());
+ }
+
+ release.countDown();
+
+ return completedFuture(null);
+ });
+
+ configuration.elements().get("name").listen(ctx -> {
+ assertNull(ctx.newValue());
+
+ log.add("deleted");
+
+ return completedFuture(null);
+ });
+
+ Future<Void> fut = configuration.change(parent ->
parent.changeElements(elements ->
Review comment:
not related to this PR but using plain `Future` in interfaces can be
considered outdated
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -231,9 +299,18 @@ public void creationErrors() throws Exception {
assertThrows(IndexOutOfBoundsException.class, () -> b.create(-1, "Z",
z -> {}));
assertThrows(IndexOutOfBoundsException.class, () -> b.create(3, "Z", z
-> {}));
- // Create after delete.
+ // Nonexisting key.
+ assertThrows(IllegalArgumentException.class, () -> b.rename("A", "Z"));
+
+ // Operations after delete.
b.delete("X");
assertThrows(IllegalArgumentException.class, () -> b.create("X", x ->
{}));
assertThrows(IllegalArgumentException.class, () -> b.create(0, "X", x
-> {}));
+ assertThrows(IllegalArgumentException.class, () -> b.rename("X", "Z"));
+ assertThrows(IllegalArgumentException.class, () -> b.rename("Y", "X"));
+
+ // Deletion of nonexistent elements doesn't break enything.
Review comment:
```suggestion
// Deletion of nonexistent elements doesn't break anything.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -82,7 +82,21 @@
NamedListChange<Change> createOrUpdate(String key, Consumer<Change>
valConsumer);
/**
- * Remove the value from named list configuration.
+ * Renames the existing value in the named list configuration.
+ *
+ * @param oldKey Key for the value to be updated.
+ * @param newKey New key for the same value.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If an element with name {@code newKey}
already exists, or an element with name
+ * {@code oldKey} doesn't exits, or {@link #delete(String)} has been
invoked with the either {@code newKey}
Review comment:
```suggestion
* {@code oldKey} doesn't exist, or {@link #delete(String)} has
previously been invoked with either the {@code newKey}
```
##########
File path:
modules/configuration-annotation-processor/src/test/java/org/apache/ignite/internal/configuration/tree/NamedListOrderTest.java
##########
@@ -17,26 +17,37 @@
package org.apache.ignite.internal.configuration.tree;
+import java.io.Serializable;
import java.util.Map;
-import org.apache.ignite.configuration.NamedListChange;
+import java.util.regex.Pattern;
import org.apache.ignite.configuration.annotation.Config;
import org.apache.ignite.configuration.annotation.ConfigurationRoot;
import org.apache.ignite.configuration.annotation.NamedConfigValue;
import org.apache.ignite.configuration.annotation.Value;
import org.apache.ignite.internal.configuration.TestConfigurationChanger;
import org.apache.ignite.internal.configuration.asm.ConfigurationAsmGenerator;
import
org.apache.ignite.internal.configuration.storage.TestConfigurationStorage;
+import org.hamcrest.Matchers;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
-import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.aMapWithSize;
+import static org.hamcrest.Matchers.anEmptyMap;
+import static org.hamcrest.Matchers.hasEntry;
+import static org.hamcrest.Matchers.hasToString;
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.matchesPattern;
import static org.junit.jupiter.api.Assertions.assertThrows;
-/** Test for keys ordering in named list nodes. */
+/** Test for named list nodes. */
public class NamedListOrderTest {
Review comment:
should we rename the test as well?
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -82,7 +82,21 @@
NamedListChange<Change> createOrUpdate(String key, Consumer<Change>
valConsumer);
/**
- * Remove the value from named list configuration.
+ * Renames the existing value in the named list configuration.
+ *
+ * @param oldKey Key for the value to be updated.
+ * @param newKey New key for the same value.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If an element with name {@code newKey}
already exists, or an element with name
+ * {@code oldKey} doesn't exits, or {@link #delete(String)} has been
invoked with the either {@code newKey}
Review comment:
> {@link #delete(String)} has been invoked with the either {@code newKey}
Why can't I rename an `oldKey` to a key that has been previously deleted?
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/NamedListChange.java
##########
@@ -82,7 +82,21 @@
NamedListChange<Change> createOrUpdate(String key, Consumer<Change>
valConsumer);
/**
- * Remove the value from named list configuration.
+ * Renames the existing value in the named list configuration.
+ *
+ * @param oldKey Key for the value to be updated.
+ * @param newKey New key for the same value.
+ * @return {@code this} for chaining.
+ *
+ * @throws NullPointerException If one of parameters is null.
+ * @throws IllegalArgumentException If an element with name {@code newKey}
already exists, or an element with name
+ * {@code oldKey} doesn't exits, or {@link #delete(String)} has been
invoked with the either {@code newKey}
+ * or {@code oldKey} previously.
+ */
+ NamedListChange<Change> rename(String oldKey, String newKey);
+
+ /**
+ * Removes the value from named list configuration.
Review comment:
```suggestion
* Removes the value from the named list configuration.
```
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
+ * not be changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is be performed when
+ *
+ * @param oldName Named, previously assigned to the element.
+ * @param newName New name of the element.
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution.
Review comment:
```suggestion
* @return Future that signifies the end of the listener execution.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -33,7 +38,10 @@
*/
public final class NamedListNode<N extends InnerNode> implements
NamedListChange<N>, TraversableTreeNode, ConstructableTreeNode {
/** Name of a synthetic configuration property that describes the order of
elements in a named list. */
- public static final String ORDER_IDX = "<idx>";
+ public static final String ORDER_IDX = "<order>";
+
+ /** */
Review comment:
This property deserves a proper javadoc
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -80,12 +88,12 @@ private NamedListNode(NamedListNode<N> node) {
/** {@inheritDoc} */
@Override public final N get(String key) {
- return map.get(key);
+ return
Optional.ofNullable(map.get(key)).map(IgniteBiTuple::getValue).orElse(null);
Review comment:
this particular `Optional` usage is considered an anti-pattern. I
personally agree with that, because it saves a single line of code here in
expense of creating additional objects
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
+ * not be changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is be performed when
+ *
+ * @param oldName Named, previously assigned to the element.
Review comment:
```suggestion
* @param oldName Name, previously assigned to the element.
```
##########
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() {
+ /**
+ * Children of the tree.
Review comment:
What do you mean by "children of the tree"? All nodes inside it?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -42,7 +50,7 @@
private final Supplier<N> valSupplier;
/** Internal container for named list element. Maps keys to named list
elements nodes. */
- private final OrderedMap<N> map;
+ private final OrderedMap<IgniteBiTuple<String, N>> map;
Review comment:
please update the javadoc, what is exactly inside this map? Also, I
think it would be better to use a named inner class instead if
`IgniteBiTuple`. We can also extract some useful methods there, like creating a
copy
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
+ * {@link #onUpdate(ConfigurationNotificationEvent)} with the only
difference that the content of the element might
+ * not be changed. No separate {@link
#onUpdate(ConfigurationNotificationEvent)} call is be performed when
+ *
+ * @param oldName Named, previously assigned to the element.
+ * @param newName New name of the element.
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution.
+ */
+ @NotNull CompletableFuture<?> onRename(String oldName, String newName,
ConfigurationNotificationEvent<VIEW> ctx);
Review comment:
why is `CompletableFuture` `NotNull` while everything else is not?
##########
File path:
modules/configuration-api/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationNamedListListener.java
##########
@@ -34,6 +34,18 @@
*/
@NotNull CompletableFuture<?>
onCreate(ConfigurationNotificationEvent<VIEW> ctx);
+ /**
+ * Called when named list element is renamed. Semantically equivalent to
Review comment:
```suggestion
* Called when a named list element is renamed. Semantically equivalent
to
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -156,18 +164,44 @@ private NamedListNode(NamedListNode<N> node) {
if (map.containsKey(key) && map.get(key) == null)
throw new IllegalArgumentException("You can't create entity that
has just been deleted [key=" + key + ']');
- N val = map.get(key);
+ IgniteBiTuple<String, N> pair = map.get(key);
- if (val == null)
- map.put(key, val = valSupplier.get());
- else
- map.put(key, val = (N)val.copy());
+ pair = pair == null
+ ? new IgniteBiTuple<>(newId(), valSupplier.get())
+ : new IgniteBiTuple<>(pair.getKey(), (N)pair.getValue().copy());
- valConsumer.accept(val);
+ map.put(key, pair);
+
+ valConsumer.accept(pair.getValue());
return this;
}
+ /** {@inheritDoc} */
+ @Override public NamedListChange<N> rename(String oldKey, String newKey) {
+ Objects.requireNonNull(oldKey, "oldKey");
+ Objects.requireNonNull(newKey, "newKey");
+
+ if (!map.containsKey(oldKey))
+ throw new IllegalArgumentException("Element with name " + oldKey +
" does not exist.");
+
+ if (map.get(oldKey) == null) {
+ throw new IllegalArgumentException(
+ "You can't rename entity that has just been deleted [key=" +
oldKey + ']'
+ );
+ }
+
+ checkNewKey(newKey);
+
+ map.rename(oldKey, newKey);
+
+ return this;
+ }
+
+ private static String newId() {
Review comment:
missing javadoc =)
##########
File path:
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/tree/OrderedMapTest.java
##########
@@ -125,9 +125,11 @@ public void rename() {
assertEquals("value2", map.get("key4"));
- assertFalse(map.containsKey("key2"));
+ assertTrue(map.containsKey("key2"));
Review comment:
why is this considered an expected behavior?
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -201,6 +235,34 @@ public String syntheticKeyName() {
return syntheticKeyName;
}
+ /**
+ * Sets internal id for the value associated with the passed key. Should
not be used in arbitrary code. Please refer
+ * to {@link ConfigurationUtil#fillFromPrefixMap(ConstructableTreeNode,
Map)} for further details on the usage.
+ *
+ * @param key Key to update. Should be present in the named list. Nothing
will happen if key is missing.
Review comment:
```suggestion
* @param key Key to update. Should be present in the named list.
Nothing will happen if the key is missing.
```
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/util/ConfigurationNotificationsUtil.java
##########
@@ -149,29 +219,32 @@ public static void notifyListeners(
/**
* Invoke {@link
ConfigurationListener#onUpdate(ConfigurationNotificationEvent)} on all passed
listeners and put
* results in {@code futures}. Not recursively.
+ *
* @param listeners List o flisteners.
* @param oldVal Old configuration value.
* @param newVal New configuration value.
* @param storageRevision Storage revision.
* @param futures Write-only list of futures.
* @param <V> Type of the node.
+ * @param <L> Type of the configuration listener.
*/
- private static <V> void notifyPublicListeners(
- List<? extends ConfigurationListener<V>> listeners,
+ private static <V, L extends ConfigurationListener<V>> void
notifyPublicListeners(
+ List<? extends L> listeners,
V oldVal,
V newVal,
long storageRevision,
- List<CompletableFuture<?>> futures
+ List<CompletableFuture<?>> futures,
+ BiFunction<L, ConfigurationNotificationEvent<V>, CompletableFuture<?>>
updater
Review comment:
this parameter is not described in the javadoc
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -201,6 +235,34 @@ public String syntheticKeyName() {
return syntheticKeyName;
}
+ /**
+ * Sets internal id for the value associated with the passed key. Should
not be used in arbitrary code. Please refer
+ * to {@link ConfigurationUtil#fillFromPrefixMap(ConstructableTreeNode,
Map)} for further details on the usage.
+ *
+ * @param key Key to update. Should be present in the named list. Nothing
will happen if key is missing.
+ * @param id New id to associate with the key.
+ */
+ public void setInternalId(String key, String id) {
+ IgniteBiTuple<String, N> pair = map.get(key);
+
+ if (pair != null)
+ pair.set1(id);
+ }
+
+ /**
+ * Returns internal id for the value associated with the passed key.
+ *
+ * @param key Key.
+ * @return Internal id.
+ *
+ * @throws IllegalArgumentException If {@code key} is not found in the
named list.
+ */
+ public String internalId(String key) {
+ return
Optional.ofNullable(map.get(key)).map(IgniteBiTuple::getKey).orElseThrow(
Review comment:
same stuff about `Optional`
##########
File path:
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/tree/NamedListNode.java
##########
@@ -201,6 +235,34 @@ public String syntheticKeyName() {
return syntheticKeyName;
}
+ /**
+ * Sets internal id for the value associated with the passed key. Should
not be used in arbitrary code. Please refer
Review comment:
```suggestion
* Sets an internal id for the value associated with the passed key.
Should not be used in arbitrary code. Refer
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
