You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Flaugh24 (via GitHub)" <gi...@apache.org> on 2023/11/15 12:29:44 UTC
[PR] IGNITE-20866 Resolved a race condition issue in the cluster configuration initialization process [ignite-3]
Flaugh24 opened a new pull request, #2844:
URL: https://github.com/apache/ignite-3/pull/2844
https://issues.apache.org/jira/browse/IGNITE-20866
Previously, the configuration was applied locally when the meta-storage revision was 0, leading to potential race conditions and the configuration being ignored if the revision changed.
Changes made:
- Altered the initialization logic to first check for the local revision of the configuration key.
- If the local revision is absent, the system now falls back to the initial configuration.
This fix ensures more reliable and consistent application of the cluster configuration, particularly during initialization phases where the meta storage revision is susceptible to change.
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20866 Resolved a race condition issue in the cluster configuration initialization process [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill commented on code in PR #2844:
URL: https://github.com/apache/ignite-3/pull/2844#discussion_r1394164260
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -321,9 +322,9 @@ private void persistDefaults() {
}
/**
- * Sets {@link #initialConfiguration}. This configuration will be used to initialize the configuration if the storage is empty. If the
- * storage is not empty, this configuration will be ignored. This method must be called before {@link #start()}. If the method is not
- * called, the initial configuration will be empty.
+ * Sets {@link #initialConfiguration}. This configuration will be used to initialize the configuration if the revision of the storage is
+ * 0. If the revision of the storage is non-zero, this configuration will be ignored. his method must be called before {@link #start()}.
Review Comment:
```suggestion
* {@code 0}. If the revision of the storage is non-zero, this configuration will be ignored. his method must be called before {@link #start()}.
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1043,18 +1038,27 @@ public void init(
}
/**
- * Initializes the cluster configuration with the specified user-provided configuration upon cluster initialization.
+ * Checks if the local revision is 0 and initializes the cluster configuration with the specified user-provided configuration upon
Review Comment:
```suggestion
* Checks if the local revision is {@code 0} and initializes the cluster configuration with the specified user-provided configuration upon
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -97,8 +97,8 @@ public abstract class ConfigurationChanger implements DynamicConfigurationChange
private volatile StorageRoots storageRoots;
/**
- * Initial configuration. This configuration will be used to initialize the configuration if the storage is empty. If the storage is not
- * empty, this configuration will be ignored.
+ * Initial configuration. This configuration will be used to initialize the configuration if the revision of the storage is 0. If the
Review Comment:
```suggestion
* Initial configuration. This configuration will be used to initialize the configuration if the revision of the storage is {@code 0}. If the
```
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/storage/ConfigurationStorage.java:
##########
@@ -79,6 +79,11 @@ public interface ConfigurationStorage extends ManuallyCloseable {
*/
CompletableFuture<Long> lastRevision();
+ /**
+ * Returns a future that will be completed with the latest revision of the storage.
+ */
Review Comment:
```suggestion
/** Returns a future that will be completed with the latest revision of the configuration storage. */
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1043,18 +1038,27 @@ public void init(
}
/**
- * Initializes the cluster configuration with the specified user-provided configuration upon cluster initialization.
+ * Checks if the local revision is 0 and initializes the cluster configuration with the specified user-provided configuration upon
+ * cluster initialization. If the local revision is not 0, does nothing.
*/
private CompletableFuture<Void> initializeClusterConfiguration(ExecutorService startupExecutor) {
- return cmgMgr.initialClusterConfigurationFuture().thenAcceptAsync(cfg -> {
- if (cfg == null) {
- return;
- }
-
- Config config = ConfigFactory.parseString(cfg);
- ConfigurationSource hoconSource = HoconConverter.hoconSource(config.root());
- clusterCfgMgr.configurationRegistry().initializeConfigurationWith(hoconSource);
- }, startupExecutor);
+ return cfgStorage.localRevision()
+ .thenComposeAsync(appliedRevision -> {
+ if (appliedRevision == 0) {
+ return completedFuture(null);
+ } else {
+ return cmgMgr.initialClusterConfigurationFuture()
+ .thenAcceptAsync(cfg -> {
Review Comment:
Just the name `cfg` doesn't say much, maybe `initialConfigHocon`?
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1043,18 +1038,27 @@ public void init(
}
/**
- * Initializes the cluster configuration with the specified user-provided configuration upon cluster initialization.
+ * Checks if the local revision is 0 and initializes the cluster configuration with the specified user-provided configuration upon
+ * cluster initialization. If the local revision is not 0, does nothing.
Review Comment:
```suggestion
* cluster initialization. If the local revision is not {@code 0}, does nothing.
```
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -265,22 +257,9 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
// - First update ever, MASTER_KEY property must be absent from MetaStorage.
// - Current node has already performed some updates or received them from MetaStorage watch listener. In this
// case "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and received updates from MetaStorage watch listeners after that. Same as
- // above, "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and have not received any updates from MetaStorage watch listeners yet.
- // In this case "curChangeId" matches MetaStorage's local revision, which may or may not match the MASTER_KEY revision. Two
- // options here:
- // - MASTER_KEY is missing in local MetaStorage copy. This means that current node have not performed nor
- // observed any configuration changes. Valid condition is "MASTER_KEY does not exist".
- // - MASTER_KEY is present in local MetaStorage copy. The MASTER_KEY revision is unknown but is less than or
- // equal to MetaStorage's local revision. Obviously, there have been no updates from the future yet. It's also guaranteed
- // that the next received configuration update will have the MASTER_KEY revision strictly greater than
- // current MetaStorage's local revision. This allows to conclude that "MASTER_KEY revision <= curChangeId" is a valid
- // condition for update.
- // Joining all of the above, it's concluded that the following condition must be used:
- Condition condition = curChangeId == 0L
+ Condition condition = changeId == 0
Review Comment:
Is it correct to delete the entire comment that was previously there?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20866 Resolved a race condition issue in the cluster configuration initialization process [ignite-3]
Posted by "tkalkirill (via GitHub)" <gi...@apache.org>.
tkalkirill merged PR #2844:
URL: https://github.com/apache/ignite-3/pull/2844
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-20866 Resolved a race condition issue in the cluster configuration initialization process [ignite-3]
Posted by "ibessonov (via GitHub)" <gi...@apache.org>.
ibessonov commented on code in PR #2844:
URL: https://github.com/apache/ignite-3/pull/2844#discussion_r1394205600
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/storage/DistributedConfigurationStorage.java:
##########
@@ -265,22 +257,9 @@ public CompletableFuture<Boolean> write(Map<String, ? extends Serializable> newV
// - First update ever, MASTER_KEY property must be absent from MetaStorage.
// - Current node has already performed some updates or received them from MetaStorage watch listener. In this
// case "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and received updates from MetaStorage watch listeners after that. Same as
- // above, "curChangeId" must match the MASTER_KEY revision exactly.
- // - Current node has been restarted and have not received any updates from MetaStorage watch listeners yet.
- // In this case "curChangeId" matches MetaStorage's local revision, which may or may not match the MASTER_KEY revision. Two
- // options here:
- // - MASTER_KEY is missing in local MetaStorage copy. This means that current node have not performed nor
- // observed any configuration changes. Valid condition is "MASTER_KEY does not exist".
- // - MASTER_KEY is present in local MetaStorage copy. The MASTER_KEY revision is unknown but is less than or
- // equal to MetaStorage's local revision. Obviously, there have been no updates from the future yet. It's also guaranteed
- // that the next received configuration update will have the MASTER_KEY revision strictly greater than
- // current MetaStorage's local revision. This allows to conclude that "MASTER_KEY revision <= curChangeId" is a valid
- // condition for update.
- // Joining all of the above, it's concluded that the following condition must be used:
- Condition condition = curChangeId == 0L
+ Condition condition = changeId == 0
Review Comment:
I'd ask to re-formulate the `Current node has already performed some updates or received them from MetaStorage watch listener`. Simple `Otherwise` would do it.
Deleted part of the comment makes no sense after the fix, this is fine
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org