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:
[email protected]
