ibessonov commented on code in PR #2066:
URL: https://github.com/apache/ignite-3/pull/2066#discussion_r1192116839
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/DistributedConfigurationUpdater.java:
##########
@@ -35,9 +43,15 @@ public
DistributedConfigurationUpdater(ClusterManagementGroupManager cmgMgr, Con
public void start() {
HoconPresentation presentation = new
HoconPresentation(clusterCfgMgr.configurationRegistry());
cmgMgr.clusterConfigurationToUpdate().thenAccept(action -> {
- if (action.currentAction() != null) {
- presentation.update(action.currentAction())
- .thenApply(v -> action.nextAction());
+ if (action.configuration() != null) {
+ presentation.update(action.configuration())
+ .handle((v, e) -> {
+ if (e != null) {
+ LOG.error("Failed to update the distributed
configuration", e);
+ }
+ return action.nextAction();
Review Comment:
Will "nextAction" nullify the configuration?
In such case, it is not applied, right? You shouldn't return the next action.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -389,25 +387,17 @@ private void onElectedAsLeader(long term) {
}
});
- raftServiceAfterJoin().thenCompose(this::pushClusterConfigToCluster);
- }
-
- private CompletableFuture<Void> pushClusterConfigToCluster(CmgRaftService
service) {
- return service.readClusterState()
- .thenCompose(state -> {
- if (state == null) {
- LOG.info("No CMG state found in the Raft service");
- return completedFuture(null);
- } else if (state.clusterConfigurationToApply() == null) {
- // Config was applied or wasn't provided
- LOG.info("No cluster configuration found in the Raft
service");
- return completedFuture(null);
- } else {
- LOG.info("Cluster configuration is found in the Raft
service, going to apply it");
- return
distributedConfigurationUpdater.updateConfiguration(state.clusterConfigurationToApply())
- .thenCompose(unused ->
removeClusterConfigFromClusterState(service));
- }
- });
+ raftServiceAfterJoin().thenAccept(service -> {
+ service.readClusterState()
+ .thenAccept(state -> {
+ updateDistributedConfigurationActionFuture.complete(
+ new UpdateDistributedConfigurationAction(
+ state.clusterConfigurationToApply(),
+
removeClusterConfigFromClusterState(service)
Review Comment:
Looks like we start deleting the configuration before it has been applied.
This is clearly a bug
##########
modules/runner/src/main/java/org/apache/ignite/internal/configuration/DistributedConfigurationUpdater.java:
##########
@@ -35,9 +43,15 @@ public
DistributedConfigurationUpdater(ClusterManagementGroupManager cmgMgr, Con
public void start() {
HoconPresentation presentation = new
HoconPresentation(clusterCfgMgr.configurationRegistry());
cmgMgr.clusterConfigurationToUpdate().thenAccept(action -> {
- if (action.currentAction() != null) {
- presentation.update(action.currentAction())
- .thenApply(v -> action.nextAction());
+ if (action.configuration() != null) {
+ presentation.update(action.configuration())
+ .handle((v, e) -> {
+ if (e != null) {
+ LOG.error("Failed to update the distributed
configuration", e);
+ }
+ return action.nextAction();
+ })
+ .thenApply(v -> v);
Review Comment:
Please explain, why we need this code. It looks out of place
--
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]
