ibessonov commented on code in PR #2301:
URL: https://github.com/apache/ignite-3/pull/2301#discussion_r1269076580
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -251,59 +258,55 @@ public void start() {
superRoot.addRoot(rootKey, rootNode);
}
- //Workaround for distributed configuration.
+ SuperRoot superRootNoDefaults = superRoot.copy();
+
+ // Workaround for distributed configuration.
Review Comment:
Is this still a workaround? I think not.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -512,16 +517,12 @@ public SuperRoot superRoot() {
* @return Future that will be completed after changes are written to the
storage.
* @throws ComponentNotStartedException if changer is not started.
*/
- private CompletableFuture<Void> changeInternally(ConfigurationSource src) {
- if (storageRoots == null) {
- throw new ComponentNotStartedException();
- }
-
+ private CompletableFuture<Void> changeInternally(ConfigurationSource src,
boolean onStartup) {
Review Comment:
Please add new parameter to javadoc
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -95,6 +94,8 @@ public abstract class ConfigurationChanger implements
DynamicConfigurationChange
/** Storage trees. */
private volatile StorageRoots storageRoots;
+ private final CompletableFuture<Void> defaultsPersisted = new
CompletableFuture<>();
Review Comment:
Please add a small javadoc. It's nice when code looks unified
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationRegistry.java:
##########
@@ -137,16 +131,10 @@ public void stop() throws Exception {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist default values to the storage.
*/
- public void initializeDefaults() {
- changer.initializeDefaults();
-
- for (RootKey<?, ?> rootKey : rootKeys) {
- DynamicConfiguration<?, ?> dynCfg = configs.get(rootKey.key());
-
- touch(dynCfg);
- }
Review Comment:
It was used in production before. This is interesting, I guess our tests are
insufficient, this sucks. I clearly remember, that there was a reason for this
code. Let's discuss it right after current issue and maybe try writing a test
that would fail without this code.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -566,7 +567,11 @@ private CompletableFuture<Void>
changeInternally0(ConfigurationSource src, long
addDefaults(changes);
- Map<String, Serializable> allChanges =
createFlattenedUpdatesMap(curRoots, changes);
+ Map<String, Serializable> allChanges =
createFlattenedUpdatesMap(localRoots.rootsWithoutDefaults, changes);
+ if (allChanges.isEmpty() && onStartup) {
+ // We don't want an empty storage update if this is the
initialization changer
Review Comment:
```suggestion
// We don't want an empty storage update if this is the
initialization changer.
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -251,59 +258,55 @@ public void start() {
superRoot.addRoot(rootKey, rootNode);
}
- //Workaround for distributed configuration.
+ SuperRoot superRootNoDefaults = superRoot.copy();
+
+ // Workaround for distributed configuration.
addDefaults(superRoot);
// Validate the restored configuration.
validateConfiguration(superRoot);
- storageRoots = new StorageRoots(superRoot, data.changeId());
+ storageRoots = new StorageRoots(superRootNoDefaults, superRoot,
data.changeId());
storage.registerConfigurationListener(configurationStorageListener());
+
+ persistDefaults();
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persists default values to the storage.
*
- * @throws ConfigurationValidationException If configuration validation
failed.
- * @throws ConfigurationChangeException If configuration framework failed
to add default values and save them to storage.
+ * <p>Specifically, this method runs a check whether there are
+ * default values that are not persisted to the storage and writes them if
there are any.
*/
- public void initializeDefaults() throws ConfigurationValidationException,
ConfigurationChangeException {
- try {
- ConfigurationSource defaultsCfgSource = new ConfigurationSource() {
- /** {@inheritDoc} */
- @Override
- public void descend(ConstructableTreeNode node) {
- addDefaults((InnerNode) node);
- }
- };
-
- changeInternally(defaultsCfgSource).get(5, TimeUnit.SECONDS);
- } catch (ExecutionException e) {
- Throwable cause = e.getCause();
-
- if (cause instanceof ConfigurationValidationException) {
- throw (ConfigurationValidationException) cause;
- }
-
- if (cause instanceof ConfigurationChangeException) {
- throw (ConfigurationChangeException) cause;
- }
-
- throw new ConfigurationChangeException(
- "Failed to write default configuration values into the
storage " + storage.getClass(), e
- );
- } catch (InterruptedException | TimeoutException e) {
- throw new ConfigurationChangeException(
- "Failed to initialize configuration storage " +
storage.getClass(), e
- );
+ private CompletableFuture<Void> persistDefaults() {
+ if (storageRoots == null) {
+ throw new ComponentNotStartedException();
Review Comment:
This is too much I think, it's a private method, and we always know who
calls 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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
