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/03/30 07:34:36 UTC
[GitHub] [ignite-3] ibessonov opened a new pull request #76: IGNITE-14180 Configuration notifications.
ibessonov opened a new pull request #76:
URL: https://github.com/apache/ignite-3/pull/76
Signed-off-by: ibessonov <be...@gmail.com>
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606121839
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -50,7 +54,7 @@
/**
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
-public class ConfigurationChanger {
+public final class ConfigurationChanger {
/** */
private final ForkJoinPool pool = new ForkJoinPool(2);
Review comment:
Well, what's the reason behind changing it then? Right now I see no pros and cons for both options)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov merged pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov merged pull request #76:
URL: https://github.com/apache/ignite-3/pull/76
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606192028
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -176,7 +200,7 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
throw new ConfigurationValidationException(validationIssues);
try {
- change(defaultsNode, storageType).get();
+ change0(defaultsNode, storageInstances.get(storageType)).get();
Review comment:
Ok
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] alievmirza commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
alievmirza commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605682179
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,26 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /**
+ * Closure interface to be used by configuration changer. Instance of this closur is passed into constructor and
Review comment:
```suggestion
* Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] SammyVimes commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
SammyVimes commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606138704
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
accidentallyget -- typo
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606191789
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
Ok, I fixed it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605462263
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -50,7 +54,7 @@
/**
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
-public class ConfigurationChanger {
+public final class ConfigurationChanger {
/** */
private final ForkJoinPool pool = new ForkJoinPool(2);
Review comment:
Should we change it then?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r604842133
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
Review comment:
This should be a proper javadoc)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -333,15 +369,25 @@ private void updateFromListener(
compressDeletedEntries(dataValuesPrefixMap);
- SuperRoot newSuperRoot = oldStorageRoots.roots.copy();
+ SuperRoot oldSuperRoot = oldStorageRoots.roots;
+ SuperRoot newSuperRoot = oldSuperRoot.copy();
fillFromPrefixMap(newSuperRoot, dataValuesPrefixMap);
- StorageRoots storageRoots = new StorageRoots(newSuperRoot, changedEntries.version());
+ StorageRoots newStorageRoots = new StorageRoots(newSuperRoot, changedEntries.cfgVersion());
+
+ storagesRootsMap.put(storageType, newStorageRoots);
+
+ ConfigurationStorage storage = storageInstances.get(storageType);
- storagesRootsMap.put(storageType, storageRoots);
+ long storegeRevision = changedEntries.storageRevision();
Review comment:
```suggestion
long storageRevision = changedEntries.storageRevision();
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
What does `null` mean in this context?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -50,7 +54,7 @@
/**
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
-public class ConfigurationChanger {
+public final class ConfigurationChanger {
/** */
private final ForkJoinPool pool = new ForkJoinPool(2);
Review comment:
BTW, why is this a ForkJoinPool and not a regular fixed thread pool?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -313,8 +339,18 @@ private void change0(
fut.completeExceptionally(new ConfigurationChangeException("Failed to change configuration", throwable));
else if (casResult)
fut.complete(null);
- else
+ else {
+ try {
+ // Is this ok to have a busy wait on concurrent configuration updates?
+ // Maybe we'll fix it while implementing metastorage storage implementation.
+ Thread.sleep(10);
+ }
+ catch (InterruptedException e) {
+ fut.completeExceptionally(e);
Review comment:
why do we have to pass the `fut` everywhere instead of using composition of CompletableFuture's? For example, instead of `pool.execute` you can use `CompletableFuture.supplyAsync` and then use `thenComposeAsync` to "flatmap` these futures
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
Review comment:
```suggestion
* @param notificator Closure to execute when update from the storage is received.
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
Can we make it a return value?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605462825
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
I mean, can this constructor be extracted as a factory method in some test class instead of polluting the API of this class?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605458226
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
Review comment:
Because this method is big and I don't want to have too much extra stuff in ConfigurationRegistry
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605512787
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
exactly
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605456127
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/storage/ConfigurationStorage.java
##########
@@ -59,4 +51,10 @@
* @param listener Listener.
*/
void removeListener(ConfigurationStorageListener listener);
+
+ /**
+ * Notify storage that this specific revision was successfully handled and there's no necessity to repeat the same
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606167400
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -50,7 +54,7 @@
/**
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
-public class ConfigurationChanger {
+public final class ConfigurationChanger {
/** */
private final ForkJoinPool pool = new ForkJoinPool(2);
Review comment:
There's no need in doing that, especially in this PR. My only concern is that `ForkJoinPool` is intended to be used with a special type of tasks, which we are not doing here, so this code might be confusing for future generations (as it has already confused me)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605453999
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -313,8 +339,18 @@ private void change0(
fut.completeExceptionally(new ConfigurationChangeException("Failed to change configuration", throwable));
else if (casResult)
fut.complete(null);
- else
+ else {
+ try {
+ // Is this ok to have a busy wait on concurrent configuration updates?
+ // Maybe we'll fix it while implementing metastorage storage implementation.
+ Thread.sleep(10);
+ }
+ catch (InterruptedException e) {
+ fut.completeExceptionally(e);
Review comment:
Because this is the future that we already returned to the user and he's waiting on it. I hope I got it right
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605464431
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
Can we return a completed future instead?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606191736
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
Yes, I inlined it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606179538
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -176,7 +200,7 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
throw new ConfigurationValidationException(validationIssues);
try {
- change(defaultsNode, storageType).get();
+ change0(defaultsNode, storageInstances.get(storageType)).get();
Review comment:
I'll rename it in the future)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606169674
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -200,6 +224,11 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
return change(superRoot, storage.getClass());
}
+ /** Stop component. */
+ public void stop() {
+ pool.shutdownNow();
Review comment:
ok!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606168220
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605510302
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
So basically you mean that I should make this one private and create new public method that will return list, right? Ok, not a problem
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605458226
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
Review comment:
Because methods in it are big and I don't want to have too much extra stuff in ConfigurationRegistry
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605451526
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
I'm not sure what you mean exactly. Can we get rid of it? Yes, we can
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606170274
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +66,26 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /**
+ * Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and
Review comment:
```suggestion
* Closure interface to be used by the configuration changer. An instance of this closure is passed into the constructor and
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -176,7 +200,7 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
throw new ConfigurationValidationException(validationIssues);
try {
- change(defaultsNode, storageType).get();
+ change0(defaultsNode, storageInstances.get(storageType)).get();
Review comment:
It's interesting that you've decided to remove the method with the name `change` and not `change0` =)
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +66,26 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /**
+ * Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and
+ * invoked every time when there's an update from any of the storages.
+ */
+ @FunctionalInterface
+ public interface Notificator {
+ /**
+ * Invoked every time when configuration is updated.
Review comment:
```suggestion
* Invoked every time when the configuration is updated.
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java
##########
@@ -508,6 +509,47 @@ public static void addDefaults(InnerNode src, InnerNode dst) {
};
}
+ /**
+ * Nullifies leaves in {@code newNode} node that are equal to corresponding leaves values in {@code curNode}.
+ * In this context we view {@code curNode} as full configuration node with all the data, while {@code newNode}
Review comment:
```suggestion
* In this context we view {@code curNode} as a full configuration node with all the data, while {@code newNode}
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +66,26 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /**
+ * Closure interface to be used by configuration changer. An instance of this closure is passed into constructor and
+ * invoked every time when there's an update from any of the storages.
+ */
+ @FunctionalInterface
+ public interface Notificator {
+ /**
+ * Invoked every time when configuration is updated.
+ * @param oldRoot Old roots values. All these roots always belong to a single storage.
+ * @param newRoot New values for the same roots as in {@code oldRoot}.
+ * @param storageRevision Revision of the storage.
+ * @return Not-null future that must signify when processing is completed. Exceptional completion is not
+ * expected.
+ */
+ @NotNull CompletableFuture<Void> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
+ }
+
+ /** Closure to execute when update from the storage is received. */
Review comment:
```suggestion
/** Closure to execute when an update from the storage is received. */
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -242,80 +271,78 @@ public SuperRoot mergedSuperRoot() {
return mergedSuperRoot;
}
- /** */
- private CompletableFuture<Void> change(SuperRoot changes, Class<? extends ConfigurationStorage> storageType) {
- ConfigurationStorage storage = storageInstances.get(storageType);
-
- CompletableFuture<Void> fut = new CompletableFuture<>();
-
- pool.execute(() -> change0(changes, storage, fut));
-
- return fut;
- }
-
/**
* Internal configuration change method that completes provided future.
* @param changes Map of changes by root key.
* @param storage Storage instance.
- * @param fut Future, that must be completed after changes are written to the storage.
+ * @return fut Future that will be completed after changes are written to the storage.
*/
- private void change0(
+ private CompletableFuture<Void> change0(
SuperRoot changes,
- ConfigurationStorage storage,
- CompletableFuture<?> fut
+ ConfigurationStorage storage
) {
StorageRoots storageRoots = storagesRootsMap.get(storage.getClass());
- SuperRoot curRoots = storageRoots.roots;
-
- Map<String, Serializable> allChanges = new HashMap<>();
-
- //TODO IGNITE-14180 single putAll + remove matching value, this way "allChanges" will be fair.
- // These are changes explicitly provided by the client.
- allChanges.putAll(nodeToFlatMap(curRoots, changes));
-
- // It is necessary to reinitialize default values every time.
- // Possible use case that explicitly requires it: creation of the same named list entry with slightly
- // different set of values and different dynamic defaults at the same time.
- SuperRoot patchedSuperRoot = patch(curRoots, changes);
- SuperRoot defaultsNode = new SuperRoot(rootKeys);
-
- addDefaults(patchedSuperRoot, defaultsNode);
-
- // These are default values for non-initialized values, required to complete the configuration.
- allChanges.putAll(nodeToFlatMap(patchedSuperRoot, defaultsNode));
-
- // Unlikely but still possible.
- if (allChanges.isEmpty()) {
- fut.complete(null);
-
- return;
- }
-
- List<ValidationIssue> validationIssues = ValidationUtil.validate(
- storageRoots.roots,
- patch(patchedSuperRoot, defaultsNode),
- this::getRootNode,
- cachedAnnotations,
- validators
- );
-
- if (!validationIssues.isEmpty()) {
- fut.completeExceptionally(new ConfigurationValidationException(validationIssues));
-
- return;
- }
-
- CompletableFuture<Boolean> writeFut = storage.write(allChanges, storageRoots.version);
-
- writeFut.whenCompleteAsync((casResult, throwable) -> {
- if (throwable != null)
- fut.completeExceptionally(new ConfigurationChangeException("Failed to change configuration", throwable));
- else if (casResult)
- fut.complete(null);
- else
- change0(changes, storage, fut);
- }, pool);
+ return CompletableFuture
+ .supplyAsync(() -> {
+ SuperRoot curRoots = storageRoots.roots;
+
+ // It is necessary to reinitialize default values every time.
+ // Possible use case that explicitly requires it: creation of the same named list entry with slightly
+ // different set of values and different dynamic defaults at the same time.
+ SuperRoot patchedSuperRoot = patch(curRoots, changes);
+
+ SuperRoot defaultsNode = new SuperRoot(rootKeys);
+
+ addDefaults(patchedSuperRoot, defaultsNode);
+
+ SuperRoot patchedChanges = patch(changes, defaultsNode);
+
+ cleanupMatchingValues(curRoots, changes);
+
+ Map<String, Serializable> allChanges = nodeToFlatMap(curRoots, patchedChanges);
+
+ // Unlikely but still possible.
+ if (allChanges.isEmpty())
+ return null;
+
+ List<ValidationIssue> validationIssues = ValidationUtil.validate(
+ storageRoots.roots,
+ patch(patchedSuperRoot, defaultsNode),
+ this::getRootNode,
+ cachedAnnotations,
+ validators
+ );
+
+ if (!validationIssues.isEmpty())
+ throw new ConfigurationValidationException(validationIssues);
+
+ return allChanges;
+ }, pool)
+ .thenCompose(allChanges -> {
+ if (allChanges == null)
+ return CompletableFuture.completedFuture(true);
+ return storage.write(allChanges, storageRoots.version)
+ .exceptionally(throwable -> {
+ throw new ConfigurationChangeException("Failed to change configuration", throwable);
+ });
+ })
+ .thenCompose(casResult -> {
+ if (casResult)
+ return CompletableFuture.completedFuture(null);
+ else {
+ try {
+ // Is this ok to have a busy wait on concurrent configuration updates?
+ // Maybe we'll fix it while implementing metastorage storage implementation.
+ Thread.sleep(10);
+ }
+ catch (InterruptedException e) {
+ return CompletableFuture.failedStage(e);
Review comment:
```suggestion
return CompletableFuture.failedFuture(e);
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java
##########
@@ -508,6 +509,47 @@ public static void addDefaults(InnerNode src, InnerNode dst) {
};
}
+ /**
+ * Nullifies leaves in {@code newNode} node that are equal to corresponding leaves values in {@code curNode}.
Review comment:
```suggestion
* Nullifies leaves in {@code newNode} node that are equal to the corresponding leaf values in {@code curNode}.
```
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606126700
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
We call this method from outside several times so I'll leave it with list parameter.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605448489
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
+ @FunctionalInterface
+ public interface Notificator {
+ /** */
+ @NotNull CompletableFuture<?> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
Review comment:
Honestly, I see no difference between these two. Should I change it?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606120594
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
Ok
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605452851
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -200,6 +224,11 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
return change(superRoot, storage.getClass());
}
+ /** Stop component. */
+ public void stop() {
+ pool.shutdownNow();
Review comment:
I know, right now it only prevents thread leaks in tests. Given that we don't have any lifecycle for our components, I'm not sure about what we should really do here. Ideally we should at least complete all the pending futures :) Let's do it later
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606125107
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
It can be inlined
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605460420
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
I think it deserves a comment
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605456379
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java
##########
@@ -508,6 +509,45 @@ public static void addDefaults(InnerNode src, InnerNode dst) {
};
}
+ /**
+ * Nullifies values in {@code newNode} that 100% match with corresponding values in {@code curNode}.
Review comment:
Ok, I will try my best to rephrase it))
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605461847
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
+ @FunctionalInterface
+ public interface Notificator {
+ /** */
+ @NotNull CompletableFuture<?> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
Review comment:
I think that the difference is that `CompletableFuture<Void>` explicitly states that there's no value inside it, while `CompletableFuture<?>` is closer to `CompletableFuture<Object>`
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605442591
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
Because I kept getting "NoClassDefFoundError" from that damn ITProcessorTest and this turned out to be the only way to fix it.
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationRegistry.java
##########
@@ -113,6 +124,38 @@ public void registerStorage(ConfigurationStorage configurationStorage) {
return changer.changeX(path, changesSource, storage);
}
+ /** */
+ public void stop() {
+ changer.stop();
+ }
+
+ /** */
+ private @NotNull CompletableFuture<?> notificator(SuperRoot oldSuperRoot, SuperRoot newSuperRoot, long storageRevision) {
+ List<CompletableFuture<?>> futures = new ArrayList<>();
+
+ newSuperRoot.traverseChildren(new ConfigurationVisitor<Void>() {
+ @Override public Void visitInnerNode(String key, InnerNode newRoot) {
+ InnerNode oldRoot = oldSuperRoot.traverseChild(key, innerNodeVisitor());
+
+ var cfg = (DynamicConfiguration<InnerNode, ?, ?>)configs.get(key);
+
+ assert oldRoot != null && cfg != null : key;
+
+ if (oldRoot != newRoot)
+ notifyListeners(oldRoot, newRoot, cfg, storageRevision, futures);
+
+ return null;
+ }
+ });
+
+ return CompletableFuture.allOf(futures.stream().map(listenerFut -> listenerFut.handle((res, throwable) -> {
Review comment:
Sure
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606121214
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
+ @FunctionalInterface
+ public interface Notificator {
+ /** */
+ @NotNull CompletableFuture<?> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
Review comment:
Ok, that makes sense. That's my laziness.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605454899
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
You mean the list? It's more convenient to accumulate values this way I think, method is recursive and I wouldn't want to merge collections every time.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606127192
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,26 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /**
+ * Closure interface to be used by configuration changer. Instance of this closur is passed into constructor and
Review comment:
Ok, thank you!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606169574
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
Can it be inlined then? =)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605464182
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
You can create a `public` method that will return a `List`, while inside you can call some recursive private methods that will populate this list. This is effectively the same approach, but I think it will make the API cleaner
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606168056
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
Writing `Future.completedFuture` is not that hard, while dealing with both nulls and normal futures looks confusing and not a very clean design. Of course, that's mostly subjective.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605453100
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -333,15 +369,25 @@ private void updateFromListener(
compressDeletedEntries(dataValuesPrefixMap);
- SuperRoot newSuperRoot = oldStorageRoots.roots.copy();
+ SuperRoot oldSuperRoot = oldStorageRoots.roots;
+ SuperRoot newSuperRoot = oldSuperRoot.copy();
fillFromPrefixMap(newSuperRoot, dataValuesPrefixMap);
- StorageRoots storageRoots = new StorageRoots(newSuperRoot, changedEntries.version());
+ StorageRoots newStorageRoots = new StorageRoots(newSuperRoot, changedEntries.cfgVersion());
+
+ storagesRootsMap.put(storageType, newStorageRoots);
+
+ ConfigurationStorage storage = storageInstances.get(storageType);
- storagesRootsMap.put(storageType, storageRoots);
+ long storegeRevision = changedEntries.storageRevision();
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605450578
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -50,7 +54,7 @@
/**
* Class that handles configuration changes, by validating them, passing to storage and listening to storage updates.
*/
-public class ConfigurationChanger {
+public final class ConfigurationChanger {
/** */
private final ForkJoinPool pool = new ForkJoinPool(2);
Review comment:
There's no reason behind it
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606169737
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
+ /**
+ * Recursively notifies all public configuration listeners, accumulating resulting futures in {@code futures} list.
+ * @param oldInnerNode Old configuration values root.
+ * @param newInnerNode New configuration values root.
+ * @param cfgNode Public configuration tree node corresponding to the current inner nodes.
+ * @param storageRevision Storage revision.
+ * @param futures Write-only list of futures to accumulate results.
+ */
+ public static void notifyListeners(
+ InnerNode oldInnerNode,
+ InnerNode newInnerNode,
+ DynamicConfiguration<InnerNode, ?, ?> cfgNode,
+ long storageRevision,
+ List<CompletableFuture<?>> futures
Review comment:
ok!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606182111
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -176,7 +200,7 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
throw new ConfigurationValidationException(validationIssues);
try {
- change(defaultsNode, storageType).get();
+ change0(defaultsNode, storageInstances.get(storageType)).get();
Review comment:
why not now?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606127006
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
That's for convenience, people will implement this method and return completed future in most cases, why not allow them to return null instead?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r606121448
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605448066
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
Review comment:
Done
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] sashapolo commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
sashapolo commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r604832296
##########
File path: modules/configuration-annotation-processor/src/main/java/org/apache/ignite/configuration/processor/internal/Processor.java
##########
@@ -394,8 +393,8 @@ private ConfigurationFieldTypes getTypes(final VariableElement field) {
final Value valueAnnotation = field.getAnnotation(Value.class);
if (valueAnnotation != null) {
- ClassName dynPropClass = ClassName.get(DynamicProperty.class);
- ClassName confValueClass = ClassName.get(ConfigurationValue.class);
+ ClassName dynPropClass = ClassName.get("org.apache.ignite.configuration.internal", "DynamicProperty");
Review comment:
Just being curious: why did you change this?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -63,6 +67,16 @@
/** Annotation classes mapped to validator objects. */
private Map<Class<? extends Annotation>, Set<Validator<?, ?>>> validators = new HashMap<>();
+ /** */
+ @FunctionalInterface
+ public interface Notificator {
+ /** */
+ @NotNull CompletableFuture<?> notify(SuperRoot oldRoot, SuperRoot newRoot, long storageRevision);
Review comment:
why not `CompletableFuture<Void>`?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationUtil.java
##########
@@ -508,6 +509,45 @@ public static void addDefaults(InnerNode src, InnerNode dst) {
};
}
+ /**
+ * Nullifies values in {@code newNode} that 100% match with corresponding values in {@code curNode}.
Review comment:
Sorry, I don't understand what you are trying to say here...
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationRegistry.java
##########
@@ -113,6 +124,38 @@ public void registerStorage(ConfigurationStorage configurationStorage) {
return changer.changeX(path, changesSource, storage);
}
+ /** */
+ public void stop() {
+ changer.stop();
+ }
+
+ /** */
+ private @NotNull CompletableFuture<?> notificator(SuperRoot oldSuperRoot, SuperRoot newSuperRoot, long storageRevision) {
+ List<CompletableFuture<?>> futures = new ArrayList<>();
+
+ newSuperRoot.traverseChildren(new ConfigurationVisitor<Void>() {
+ @Override public Void visitInnerNode(String key, InnerNode newRoot) {
+ InnerNode oldRoot = oldSuperRoot.traverseChild(key, innerNodeVisitor());
+
+ var cfg = (DynamicConfiguration<InnerNode, ?, ?>)configs.get(key);
+
+ assert oldRoot != null && cfg != null : key;
+
+ if (oldRoot != newRoot)
+ notifyListeners(oldRoot, newRoot, cfg, storageRevision, futures);
+
+ return null;
+ }
+ });
+
+ return CompletableFuture.allOf(futures.stream().map(listenerFut -> listenerFut.handle((res, throwable) -> {
Review comment:
Please extract the inner stream into a variable, it's hard to read otherwise
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -86,8 +100,18 @@ private StorageRoots(SuperRoot roots, long version) {
/** Storage instances by their classes. Comes in handy when all you have is {@link RootKey}. */
private final Map<Class<? extends ConfigurationStorage>, ConfigurationStorage> storageInstances = new HashMap<>();
- /** Constructor. */
- public ConfigurationChanger(RootKey<?, ?>... rootKeys) {
+ /**
+ * @param notificator Closure to execute when update forom storage is received.
+ */
+ public ConfigurationChanger(Notificator notificator) {
+ this.notificator = notificator;
+ }
+
+ /** Constructor for tests. */
+ @TestOnly
+ ConfigurationChanger(RootKey<?, ?>... rootKeys) {
Review comment:
Can it be extracted somewhere?
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/storage/ConfigurationStorage.java
##########
@@ -59,4 +51,10 @@
* @param listener Listener.
*/
void removeListener(ConfigurationStorageListener listener);
+
+ /**
+ * Notify storage that this specific revision was successfully handled and there's no necessity to repeat the same
Review comment:
```suggestion
* Notify storage that this specific revision was successfully handled and it is not necessary to repeat the same
```
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/internal/util/ConfigurationNotificationsUtil.java
##########
@@ -0,0 +1,184 @@
+/*
+ * 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.configuration.internal.util;
+
+import java.io.Serializable;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.CompletableFuture;
+import java.util.stream.Collectors;
+import org.apache.ignite.configuration.ConfigurationProperty;
+import org.apache.ignite.configuration.internal.DynamicConfiguration;
+import org.apache.ignite.configuration.internal.DynamicProperty;
+import org.apache.ignite.configuration.internal.NamedListConfiguration;
+import org.apache.ignite.configuration.internal.notifications.ConfigurationNotificationEventImpl;
+import org.apache.ignite.configuration.notifications.ConfigurationListener;
+import org.apache.ignite.configuration.notifications.ConfigurationNotificationEvent;
+import org.apache.ignite.configuration.tree.ConfigurationVisitor;
+import org.apache.ignite.configuration.tree.InnerNode;
+import org.apache.ignite.configuration.tree.NamedListNode;
+import org.apache.ignite.configuration.tree.NamedListView;
+
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.innerNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.leafNodeVisitor;
+import static org.apache.ignite.configuration.internal.util.ConfigurationUtil.namedListNodeVisitor;
+
+/** */
+public class ConfigurationNotificationsUtil {
Review comment:
why do you need this class? It's only used in `ConfigurationRegistry`
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/ConfigurationChanger.java
##########
@@ -200,6 +224,11 @@ public void initialize(Class<? extends ConfigurationStorage> storageType) {
return change(superRoot, storage.getClass());
}
+ /** Stop component. */
+ public void stop() {
+ pool.shutdownNow();
Review comment:
AFAIK, this is not a good way to shutdown an executor. Usually it is done in a much more complex way (see [guava](https://github.com/google/guava/blob/master/guava/src/com/google/common/util/concurrent/MoreExecutors.java#L1068) for example)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ibessonov commented on a change in pull request #76: IGNITE-14180 Configuration notifications.
Posted by GitBox <gi...@apache.org>.
ibessonov commented on a change in pull request #76:
URL: https://github.com/apache/ignite-3/pull/76#discussion_r605455165
##########
File path: modules/configuration/src/main/java/org/apache/ignite/configuration/notifications/ConfigurationListener.java
##########
@@ -0,0 +1,38 @@
+/*
+ * 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.configuration.notifications;
+
+import java.util.concurrent.CompletableFuture;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * Configuration property change listener.
+ *
+ * @param <VIEW> VIEW type configuration.
+ */
+@FunctionalInterface
+public interface ConfigurationListener<VIEW> {
+ /**
+ * Called on property value update.
+ *
+ * @param ctx Notification context.
+ * @return Future that signifies end of listener execution. Can be {@code null}.
Review comment:
null is equivalent to completed future. I'll clarify it in the comment.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org