ibessonov commented on code in PR #2301:
URL: https://github.com/apache/ignite-3/pull/2301#discussion_r1267739789
##########
modules/client/src/test/java/org/apache/ignite/client/TestServer.java:
##########
@@ -125,6 +125,7 @@ public TestServer(
);
cfg.start();
+ cfg.persistDefaults().join();
Review Comment:
Please use `assertThat(cfg.persistDefaults(), willCompleteSuccessfully());`.
There should be no joins without timeouts, with the exception of already
completed futures.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -251,9 +253,15 @@ public void start() {
superRoot.addRoot(rootKey, rootNode);
}
+ SuperRoot superRootNoDefaults = superRoot.copy();
+
//Workaround for distributed configuration.
Review Comment:
```suggestion
// Workaround for distributed configuration.
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -251,9 +253,15 @@ public void start() {
superRoot.addRoot(rootKey, rootNode);
}
+ SuperRoot superRootNoDefaults = superRoot.copy();
+
//Workaround for distributed configuration.
addDefaults(superRoot);
+ //This map contains properties that are set as 'defaults' from the
code and are missing in the storage.
+ Map<String, Serializable> allChanges =
createFlattenedUpdatesMap(superRootNoDefaults, superRoot);
+ defaultChanges = allChanges.isEmpty() ? null : allChanges;
Review Comment:
I wonder why you made it nullable. Was having an empty map not enough?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -263,40 +271,63 @@ public void start() {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist 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>Make sure all properties that have their values set in the schema
defaults are persisted to the storage.
+ * The goal is to keep all configuration values in onl place
+ * and avoid possible cluster inconsistency when code defaults change in
the future releases.
+ *
+ * @return Future that will be completed after changes have been written
to the storage.
*/
- 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();
+ public CompletableFuture<Void> persistDefaults() {
+ if (storageRoots == null) {
+ throw new ComponentNotStartedException();
+ }
+ if (defaultChanges == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ return storage.lastRevision()
+ .thenComposeAsync(this::persistDefaultsInternal, pool)
+ .thenRun(() -> defaultChanges = null)
Review Comment:
Can `persistDefaultsInternal` do this?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -263,40 +271,63 @@ public void start() {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist 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>Make sure all properties that have their values set in the schema
defaults are persisted to the storage.
+ * The goal is to keep all configuration values in onl place
Review Comment:
```suggestion
* The goal is to keep all configuration values in one place
```
##########
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:
Why have you removed this code? I believe it may be necessary for
notifications to work properly, but I have no tests to support the statement.
Did you fully understand the intention of the code before removing it?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/ddl/DdlCommandHandlerExceptionHandlingTest.java:
##########
@@ -100,8 +100,6 @@ public class DdlCommandHandlerExceptionHandlingTest extends
IgniteAbstractTest {
void before() {
registry.start();
- registry.initializeDefaults();
Review Comment:
Same question here
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/DistributionZoneManagerTest.java:
##########
@@ -92,8 +92,6 @@ public void setUp() {
registry.start();
- registry.initializeDefaults();
Review Comment:
Right after I said _"they are always called together"_, I see that they're
not. Is this intentional, or you simply forgot to call the method?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -263,40 +271,63 @@ public void start() {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist default values to the storage.
Review Comment:
```suggestion
* Persists default values to the storage.
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -263,40 +271,63 @@ public void start() {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist 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>Make sure all properties that have their values set in the schema
defaults are persisted to the storage.
Review Comment:
```suggestion
* <p>Makes sure that all properties that have their values set in the
schema defaults are persisted to the storage.
```
##########
modules/configuration-presentation/src/test/java/org/apache/ignite/internal/configuration/presentation/HoconPresentationTest.java:
##########
@@ -90,6 +90,7 @@ public void validate(Value annotation,
ValidationContext<Object> ctx) {
);
cfgRegistry.start();
+ cfgRegistry.persistDefaults().get(1, SECONDS);
Review Comment:
I suggest using `assertThat(..., willCompleteSuccessfully())` in all such
places as well, this will make code more unified one day
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/ItClusterManagerTest.java:
##########
@@ -385,10 +384,10 @@ void
testClusterConfigurationIsRemovedFromClusterStateAfterUpdating(TestInfo tes
.until(() -> findLeaderNode(cluster), Optional::isPresent)
.get();
- // Check the new leader cancels the action.
+ // Check the new leader completes with an empty action
Review Comment:
```suggestion
// Check the new leader completes with an empty action.
```
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -131,6 +131,7 @@ public static void afterAll() {
public void testSimpleConfigurationChange() throws Exception {
ConfigurationChanger changer = createChanger(KEY);
changer.start();
+ changer.persistDefaults().get(1, SECONDS);
Review Comment:
Please use `assertThat(..., willCompleteSuccessfully())`, it looks nicer.
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/tree/InternalIdTest.java:
##########
@@ -98,8 +98,7 @@ void setUp() {
);
registry.start();
-
- registry.initializeDefaults();
+ registry.persistDefaults().get(1, TimeUnit.SECONDS);
Review Comment:
Why don't we move this call inside of the `start()`? It may return the
future as well. I see that they are always called together, that's a good
argument for combining them.
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -263,40 +271,63 @@ public void start() {
}
/**
- * Initializes the configuration storage - reads data and sets default
values for missing configuration properties.
+ * Persist 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>Make sure all properties that have their values set in the schema
defaults are persisted to the storage.
+ * The goal is to keep all configuration values in onl place
+ * and avoid possible cluster inconsistency when code defaults change in
the future releases.
+ *
+ * @return Future that will be completed after changes have been written
to the storage.
*/
- 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();
+ public CompletableFuture<Void> persistDefaults() {
+ if (storageRoots == null) {
+ throw new ComponentNotStartedException();
+ }
+ if (defaultChanges == null) {
+ return CompletableFuture.completedFuture(null);
+ }
+ return storage.lastRevision()
+ .thenComposeAsync(this::persistDefaultsInternal, pool)
+ .thenRun(() -> defaultChanges = null)
+ .exceptionally(throwable -> {
+ Throwable cause = throwable.getCause();
+
+ if (cause instanceof ConfigurationChangeException) {
+ throw (ConfigurationChangeException) cause;
+ } else {
+ throw new ConfigurationChangeException("Failed to
persist defaults", cause);
+ }
+ });
+ }
- if (cause instanceof ConfigurationValidationException) {
- throw (ConfigurationValidationException) cause;
+ /**
+ * Internal configuration persist method.
+ */
+ private CompletableFuture<Void> persistDefaultsInternal(long
storageRevision) {
Review Comment:
I have doubts in the approach you've chosen. Remember that `//Workaround for
distributed configuration.` comment in the code? It's there for a reason. And
the reason is - it's a workaround, that must be removed. I believe it must be
removed in this issue. Sorry for not having proper TODO with the description in
that comment, that's on me. Let me explain my position.
Now, this is going to be a pretty long comment, so bear with me.
1. Can this CAS fail? I see that literally every node tries to execute the
operation.So it seems like it can. On the other hand, incoming updates should
make the map empty. Can we have a concurrent update of the configuration, that
will come from a user? Maybe. You probably meant the upgrade from older to a
newer version of Ignite.
Are there plans for `init` operation upon upgrade, or we will just quietly
add default, invisible to the user? This is a complicated question.
2. I think there might be a simpler way to do things, it's removing that
code with a "workaround" comment. This way, all changers will always explicitly
write data, that was previously `null`, to the storage (with some tweaks).
There are advantages to such approach:
- Upon init, we would only need a single configuration update, instead of
two.
- We will reuse already existing code, making the procedure much simpler.
One thing to consider - we may still need to call similar method on each
start, but having the update with empty map, that will have a specific property
of not incrementing the storage revision if there are no updates, would solve
the issue, with much less complexity in code.
The only possible issue here, that I see, is that some validators would get
`null` from "old" values of configuration, and that must be handled properly.
This is also a little inconvenient, because primitive values would throw
`NullPointerException` instead of returning nulls, because you know, primitives
can't be nulls. This issue can be eliminated, if we will have both "tree with
defaults" and "tree without defaults" in the `storageRoots`.
Otherwise, it seems like a better approach to me.
3. Maybe this is not relevant, but this is, most likely, a misuse of the
read lock. It has a very specific purpose of protecting a single field. I guess
it's not documented properly. But the idea is - it protected the state of the
`storageRoots` field while executing `src.descend(changes);`. Your code doesn't
use any external closures, and the only access you have to `storageRoots` is a
single volatile read, which would still be correct without the read lock.
Considering that I propose deleting the method entirely, this comment was
purely informational.
Overall, I would like to discuss my alternative approach with you. Let's do
it, please also ask clarifying questions about what I've said, it may be a
little confusing, I know that.
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -275,7 +282,10 @@ public void testFailedToWrite() {
CompletableFuture<Map<String, ? extends Serializable>> dataFuture =
storage.readDataOnRecovery().thenApply(Data::values);
- assertThat(dataFuture, willBe(anEmptyMap()));
+ //these are the default values taken from FirstConfigurationSchema
Review Comment:
```suggestion
// These are the default values taken from FirstConfigurationSchema.
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -251,9 +253,15 @@ public void start() {
superRoot.addRoot(rootKey, rootNode);
}
+ SuperRoot superRootNoDefaults = superRoot.copy();
+
//Workaround for distributed configuration.
Review Comment:
I know that's my comment, but can you please add that space that I missed?
Thank you!
Please do the same for the next comment as well.
--
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]
