ibessonov commented on code in PR #2817:
URL: https://github.com/apache/ignite-3/pull/2817#discussion_r1387924545
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -239,7 +241,7 @@ public class IgniteImpl implements Ignite {
private final ConfigurationManager clusterCfgMgr;
/** Cluster configuration defaults setter. */
Review Comment:
Please update the javadoc as well
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/ItClusterManagerTest.java:
##########
@@ -102,6 +99,26 @@ void testInit(TestInfo testInfo) throws Exception {
assertThat(cluster.get(1).logicalTopologyNodes(),
will(containsInAnyOrder(expectedTopology)));
}
+ /**
+ * Tests initial cluster setup with provided configuration.
+ */
+ @Test
+ void testInitWithProvidedConfiguration(TestInfo testInfo) throws Exception
{
+ startCluster(3, testInfo);
+
+ String[] cmgNodes = { cluster.get(0).name() };
+
+ String[] metaStorageNodes = { cluster.get(1).name() };
+
+ String configuration = "cluster-configuration.json";
Review Comment:
It's not a file name, it is? If it doesn't pass validation, how do we expect
this test to work?
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -295,6 +310,17 @@ private void persistDefaults() {
});
}
+ /**
+ * Initializes the configuration with the given source. This method should
be used only for the initial setup of the configuration. The
+ * configuration is initialized with the provided source only if the
storage is empty, and it is saved along with the defaults. This
+ * method must be called before {@link #start()}.
+ *
+ * @param cfg the configuration source to initialize with.
+ */
+ public void initializeConfigurationWith(ConfigurationSource cfg) {
+ initialConfiguration = cfg;
Review Comment:
This method must be called before `start`, right? I believe we should
thoroughly document the flow and add necessary assertions. For example, I see
the `initialConfiguration != null` check in this class. Is this possible?
There's no info about nullability. Please add info about it and clarify that
it's a "lazy-init" field, meaning that it's only set once, but not in
constructor.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -356,6 +355,10 @@ private ClusterState createClusterState(CmgInitMessage
msg) {
private void onElectedAsLeader(long term) {
LOG.info("CMG leader has been elected, executing onLeaderElected
callback");
+ raftServiceAfterJoin()
+ .thenCompose(CmgRaftService::readClusterState)
+ .thenAccept(state ->
initialClusterConfigurationFuture.complete(state.initialClusterConfiguration()));
Review Comment:
Please comment, why is this important. We clearly complete the same future
in `handleClusterState`, right?
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -239,7 +241,7 @@ public class IgniteImpl implements Ignite {
private final ConfigurationManager clusterCfgMgr;
/** Cluster configuration defaults setter. */
- private final ConfigurationDynamicDefaultsPatcherImpl
clusterConfigurationDefaultsSetter;
+ private final ConfigurationDynamicDefaultsPatcherImpl
configurationDynamicDefaultsPatcher;
Review Comment:
Why are we declaring the field as `*Impl` anyway, is that intentional?
##########
modules/configuration/src/test/java/org/apache/ignite/internal/configuration/ConfigurationChangerTest.java:
##########
@@ -568,6 +570,26 @@ void testUpdateStorageRevisionOnEmptyChanges() throws
Exception {
assertEquals(3, storage.lastRevision().get(1, SECONDS));
}
+ @Test
+ public void initializeWith() throws Exception {
+ // When we initialize the configuration with a source, the
configuration should be updated with the values from the source.
+ String initialConfiguration =
"def:{child:{defStr:initialStr,arr:[bar]}}";
+ com.typesafe.config.Config config =
ConfigFactory.parseString(initialConfiguration);
+ ConfigurationSource hoconSource =
HoconConverter.hoconSource(config.root());
+
+ var changer = createChanger(DefaultsConfiguration.KEY);
+
+ changer.initializeConfigurationWith(hoconSource);
+
+ changer.start();
+
+ DefaultsView root = (DefaultsView)
changer.getRootNode(DefaultsConfiguration.KEY);
+
+ assertEquals("foo", root.defStr());
+ assertEquals("initialStr", root.child().defStr());
+ assertEquals(List.of("bar"), Arrays.asList(root.child().arr()));
Review Comment:
I believe there's an `assertArrayEquals` somewhere in JUnit
##########
modules/configuration/src/main/java/org/apache/ignite/internal/configuration/ConfigurationChanger.java:
##########
@@ -295,6 +310,17 @@ private void persistDefaults() {
});
}
+ /**
+ * Initializes the configuration with the given source. This method should
be used only for the initial setup of the configuration. The
+ * configuration is initialized with the provided source only if the
storage is empty, and it is saved along with the defaults. This
+ * method must be called before {@link #start()}.
+ *
+ * @param cfg the configuration source to initialize with.
+ */
+ public void initializeConfigurationWith(ConfigurationSource cfg) {
+ initialConfiguration = cfg;
Review Comment:
I see more comments in the registry class, but when it comes to the
implementation, `changer` should have enough comments to understand everything
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -1049,8 +1044,23 @@ public void init(
}
/**
- * Recovers components state on start by invoking configuration listeners
({@link #notifyConfigurationListeners()}
- * and deploying watches after that.
+ * Initializes the cluster configuration with the specified user-provided
configuration upon cluster initialization.
+ */
+ private CompletableFuture<Void>
initializeClusterConfiguration(ExecutorService startupExecutor) {
+ return cmgMgr.initialClusterConfigurationFuture().thenAcceptAsync(cfg
-> {
+ if (cfg == null) {
+ return;
+ }
Review Comment:
I would recommend using EMPTY_CFG_SOURCE (or whatever that name is), it
would simplify assertions in configuration changer.
##########
modules/runner/src/main/java/org/apache/ignite/internal/app/IgniteImpl.java:
##########
@@ -803,6 +797,14 @@ public CompletableFuture<Ignite> start(Path configPath) {
return metaStorageMgr.recoveryFinishedFuture();
}, startupExecutor)
+ .thenComposeAsync(revision -> {
Review Comment:
What if the entire lambda becomes `initializeClusterConfiguration`? Less
lines, easier to read...
--
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]
