sashapolo commented on code in PR #5315:
URL: https://github.com/apache/ignite-3/pull/5315#discussion_r1983210856
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -107,29 +107,44 @@ public CompletableFuture<Void> initCluster(
String clusterName,
@Nullable String clusterConfiguration
) {
- if (metaStorageNodeNames.isEmpty()) {
- throw new IllegalArgumentException("Meta Storage node names list
must not be empty");
- }
-
if (metaStorageNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("Meta Storage node names must
not contain blank strings: " + metaStorageNodeNames);
}
- if (!cmgNodeNames.isEmpty() &&
cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
+ Set<String> msNodeNameSet =
metaStorageNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (msNodeNameSet.size() != metaStorageNodeNames.size()) {
+ throw new IllegalArgumentException("Meta Storage node names must
not contain duplicates: " + metaStorageNodeNames);
+ }
+
+ if (cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("CMG node names must not
contain blank strings: " + cmgNodeNames);
}
+ Set<String> cmgNodeNameSet =
cmgNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (cmgNodeNameSet.size() != cmgNodeNames.size()) {
+ throw new IllegalArgumentException("CMG node names must not
contain duplicates: " + metaStorageNodeNames);
+ }
+
+
+ if (msNodeNameSet.isEmpty() && cmgNodeNameSet.isEmpty()) {
+ var clusterNodes = clusterService.topologyService().allMembers();
Review Comment:
All these `var` usages are against the code style, please remove them
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -107,29 +107,44 @@ public CompletableFuture<Void> initCluster(
String clusterName,
@Nullable String clusterConfiguration
) {
- if (metaStorageNodeNames.isEmpty()) {
- throw new IllegalArgumentException("Meta Storage node names list
must not be empty");
- }
-
if (metaStorageNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("Meta Storage node names must
not contain blank strings: " + metaStorageNodeNames);
}
- if (!cmgNodeNames.isEmpty() &&
cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
+ Set<String> msNodeNameSet =
metaStorageNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (msNodeNameSet.size() != metaStorageNodeNames.size()) {
+ throw new IllegalArgumentException("Meta Storage node names must
not contain duplicates: " + metaStorageNodeNames);
+ }
+
+ if (cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("CMG node names must not
contain blank strings: " + cmgNodeNames);
}
+ Set<String> cmgNodeNameSet =
cmgNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (cmgNodeNameSet.size() != cmgNodeNames.size()) {
+ throw new IllegalArgumentException("CMG node names must not
contain duplicates: " + metaStorageNodeNames);
+ }
+
+
Review Comment:
```suggestion
```
##########
modules/runner/src/test/java/org/apache/ignite/InitParametersBuilderTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.junit.jupiter.api.Test;
+
+class InitParametersBuilderTest extends BaseIgniteAbstractTest {
+
+ @Test
+ void build_WithAllParametersSet_ReturnsExpectedInitParameters() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ String clusterName = "TestCluster";
+ List<String> metaStorageNodes = List.of("Node1", "Node2");
+ List<String> cmgNodes = List.of("Node3", "Node4");
+ String clusterConfiguration = "{config: value}";
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames(metaStorageNodes)
+ .cmgNodeNames(cmgNodes)
+ .clusterName(clusterName)
+ .clusterConfiguration(clusterConfiguration)
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertEquals(metaStorageNodes, result.metaStorageNodeNames());
+ assertEquals(cmgNodes, result.cmgNodeNames());
+ assertEquals(clusterName, result.clusterName());
+ assertEquals(clusterConfiguration, result.clusterConfiguration());
+ }
+
+ @Test
+ void build_WithoutClusterName_ThrowsIllegalStateException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ IllegalStateException exception =
assertThrows(IllegalStateException.class, builder::build);
+ assertEquals("Cluster name is not set.", exception.getMessage());
+ }
+
+ @Test
+ void build_WithNullClusterName_ThrowsIllegalArgumentException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ //noinspection DataFlowIssue
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> builder.clusterName(null));
+ assertEquals("Cluster name cannot be null or empty.",
exception.getMessage());
+ }
+
+ @Test
+ void build_WithBlankClusterName_ThrowsIllegalArgumentException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> builder.clusterName(" "));
+ assertEquals("Cluster name cannot be null or empty.",
exception.getMessage());
+ }
+
+ @Test
+ void build_WithEmptyMetaStorageNodeNames_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames()
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.metaStorageNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithEmptyCmgNodeNames_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .cmgNodeNames()
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.cmgNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithNullMetaStorageNodeCollection_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames((List<String>) null)
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.metaStorageNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithNullCmgNodeCollection_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .cmgNodeNames((List<String>) null)
Review Comment:
Similar to my comment above, I think this is very weird and we should throw
an exception in this case.
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/commands/cluster/ClusterInitTest.java:
##########
@@ -204,18 +229,28 @@ void initError() {
@Test
@DisplayName("--url http://localhost:10300 --cluster-management-group
node2ConsistentId, node3ConsistentId")
- void metastorageNodesAreMandatoryForInit() {
+ void metastorageNodesAreNotMandatoryForInit() {
Review Comment:
What is the expected behavior here? Are we going to choose meta storage
nodes automatically while using the provided CMG nodes? Or are they going to be
the same?
##########
docs/_docs/administrators-guide/lifecycle.adoc:
##########
@@ -40,7 +40,15 @@ First, the nodes specified in the
`--cluster-management-group` argument form a R
image::images/lifecycle1.png[Cluster initialization]
-Then, the nodes specified in the `--metastorage-group` argument form a RAFT
group and take the role of *metastorage group*. These nodes will hold the
authoritative copy of cluster meta information.
+Then, the nodes specified in the `--metastorage-group` argument form a RAFT
group and take the role of *metastorage group*. These nodes will hold the
authoritative copy of cluster meta information. If `--metastorage-group` is not
specified, the nodes are chosen automatically. The number of nodes is chosen
automatically as follows:
+
+[source, javascript]
+----
+metastorage_group_size =
+ cluster_size <= 3 ? cluster_size // If up to 3 nodes, use all nodes for
CMG and metastorage.
+ : cluster_size == 4 ? 3 // If 4 nodes, use 3 nodes to have an odd number
of nodes for better split-brain protection.
Review Comment:
> for better split-brain protection
What do you mean here? How can there be a split-brain situation?
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/call/cluster/ClusterInitCallInput.java:
##########
@@ -35,8 +35,8 @@ public class ClusterInitCallInput implements CallInput {
private ClusterInitCallInput(
String clusterUrl,
- List<String> metaStorageNodes,
- List<String> cmgNodes,
+ @Nullable List<String> metaStorageNodes,
Review Comment:
Can we use empty lists instead of `null`?
##########
modules/runner/src/test/java/org/apache/ignite/InitParametersBuilderTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.junit.jupiter.api.Test;
+
+class InitParametersBuilderTest extends BaseIgniteAbstractTest {
+
+ @Test
+ void build_WithAllParametersSet_ReturnsExpectedInitParameters() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ String clusterName = "TestCluster";
+ List<String> metaStorageNodes = List.of("Node1", "Node2");
+ List<String> cmgNodes = List.of("Node3", "Node4");
+ String clusterConfiguration = "{config: value}";
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames(metaStorageNodes)
+ .cmgNodeNames(cmgNodes)
+ .clusterName(clusterName)
+ .clusterConfiguration(clusterConfiguration)
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertEquals(metaStorageNodes, result.metaStorageNodeNames());
+ assertEquals(cmgNodes, result.cmgNodeNames());
+ assertEquals(clusterName, result.clusterName());
+ assertEquals(clusterConfiguration, result.clusterConfiguration());
+ }
+
+ @Test
+ void build_WithoutClusterName_ThrowsIllegalStateException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ IllegalStateException exception =
assertThrows(IllegalStateException.class, builder::build);
+ assertEquals("Cluster name is not set.", exception.getMessage());
+ }
+
+ @Test
+ void build_WithNullClusterName_ThrowsIllegalArgumentException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ //noinspection DataFlowIssue
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> builder.clusterName(null));
+ assertEquals("Cluster name cannot be null or empty.",
exception.getMessage());
+ }
+
+ @Test
+ void build_WithBlankClusterName_ThrowsIllegalArgumentException() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act & Assert
+ IllegalArgumentException exception =
assertThrows(IllegalArgumentException.class, () -> builder.clusterName(" "));
+ assertEquals("Cluster name cannot be null or empty.",
exception.getMessage());
+ }
+
+ @Test
+ void build_WithEmptyMetaStorageNodeNames_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames()
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.metaStorageNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithEmptyCmgNodeNames_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .cmgNodeNames()
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.cmgNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithNullMetaStorageNodeCollection_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodeNames((List<String>) null)
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.metaStorageNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithNullCmgNodeCollection_SetsEmptyList() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+
+ // Act
+ InitParameters result = builder
+ .cmgNodeNames((List<String>) null)
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertTrue(result.cmgNodeNames().isEmpty());
+ }
+
+ @Test
+ void build_WithNullClusterConfiguration_SetsNull() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ String clusterName = "TestCluster";
+
+ // Act
+ InitParameters result = builder
+ .clusterName(clusterName)
+ .clusterConfiguration(null)
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertNull(result.clusterConfiguration());
+ }
+
+ @Test
+ void build_WithIgniteServerMetaStorageNodes_ReturnsExpectedNames() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ IgniteServer server1 = mock(IgniteServer.class);
+ IgniteServer server2 = mock(IgniteServer.class);
+
+ when(server1.name()).thenReturn("Node1");
+ when(server2.name()).thenReturn("Node2");
+
+ // Act
+ InitParameters result = builder
+ .metaStorageNodes(server1, server2)
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertEquals(List.of("Node1", "Node2"), result.metaStorageNodeNames());
+ }
+
+ @Test
+ void build_WithIgniteServerCmgNodes_ReturnsExpectedNames() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ IgniteServer server1 = mock(IgniteServer.class);
+ IgniteServer server2 = mock(IgniteServer.class);
+
+ when(server1.name()).thenReturn("Node3");
+ when(server2.name()).thenReturn("Node4");
+
+ // Act
+ InitParameters result = builder
+ .cmgNodes(server1, server2)
+ .clusterName("TestCluster")
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertEquals(List.of("Node3", "Node4"), result.cmgNodeNames());
+ }
+
+ @Test
+ void build_WithOnlyClusterName_SetsDefaults() {
+ // Arrange
+ InitParametersBuilder builder = new InitParametersBuilder();
+ String clusterName = "TestCluster";
+
+ // Act
+ InitParameters result = builder
+ .clusterName(clusterName)
+ .build();
+
+ // Assert
+ assertNotNull(result);
+ assertEquals(clusterName, result.clusterName());
+ assertTrue(result.metaStorageNodeNames().isEmpty(), "Meta storage node
names should be empty by default.");
+ assertTrue(result.cmgNodeNames().isEmpty(), "CMG node names should be
empty by default.");
+ assertNull(result.clusterConfiguration(), "Cluster configuration
should be null by default.");
+ }
+}
Review Comment:
Please configure your Idea to add line breaks at the end of files
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -107,29 +107,44 @@ public CompletableFuture<Void> initCluster(
String clusterName,
@Nullable String clusterConfiguration
) {
- if (metaStorageNodeNames.isEmpty()) {
- throw new IllegalArgumentException("Meta Storage node names list
must not be empty");
- }
-
if (metaStorageNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("Meta Storage node names must
not contain blank strings: " + metaStorageNodeNames);
}
- if (!cmgNodeNames.isEmpty() &&
cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
+ Set<String> msNodeNameSet =
metaStorageNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (msNodeNameSet.size() != metaStorageNodeNames.size()) {
+ throw new IllegalArgumentException("Meta Storage node names must
not contain duplicates: " + metaStorageNodeNames);
+ }
+
+ if (cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("CMG node names must not
contain blank strings: " + cmgNodeNames);
}
+ Set<String> cmgNodeNameSet =
cmgNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (cmgNodeNameSet.size() != cmgNodeNames.size()) {
+ throw new IllegalArgumentException("CMG node names must not
contain duplicates: " + metaStorageNodeNames);
+ }
+
+
+ if (msNodeNameSet.isEmpty() && cmgNodeNameSet.isEmpty()) {
+ var clusterNodes = clusterService.topologyService().allMembers();
+ var topologySize = clusterNodes.size();
+ var numberOfMsNodes = topologySize < 5 ? 3 : 5;
+ var chosenNodes =
clusterNodes.stream().map(ClusterNode::name).sorted().limit(numberOfMsNodes)
Review Comment:
I don't know if this is a problem or not, but you specified that sorting is
needed to achieve predictable outcome when running init multiple times. But
`topology` service returns a local view of the topology on a particular node,
so the outcome may be different depending on timing.
##########
modules/runner/src/test/java/org/apache/ignite/InitParametersBuilderTest.java:
##########
@@ -0,0 +1,231 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.ignite;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.util.List;
+import org.apache.ignite.internal.testframework.BaseIgniteAbstractTest;
+import org.junit.jupiter.api.Test;
+
+class InitParametersBuilderTest extends BaseIgniteAbstractTest {
Review Comment:
Kudos for the thorough testing =)
##########
modules/runner/src/main/java/org/apache/ignite/InitParameters.java:
##########
@@ -45,17 +46,15 @@ public class InitParameters {
* @param clusterConfiguration Cluster configuration.
*/
InitParameters(
- Collection<String> metaStorageNodeNames,
- Collection<String> cmgNodeNames,
+ @Nullable Collection<String> metaStorageNodeNames,
Review Comment:
Again, I would suggest to use empty collections by default, instead of `null`
##########
modules/cluster-management/src/test/java/org/apache/ignite/internal/cluster/management/ClusterInitializerTest.java:
##########
@@ -112,6 +118,59 @@ void testNormalInit() {
assertThat(initFuture, willBe(nullValue(Void.class)));
}
+ @ParameterizedTest
+ @ValueSource(ints = {1, 2, 3, 4, 5, 9}) // Runs the test with 1 to 10
nodes
+ void testInitEmptyMsCmgNodes(int numNodes) {
+ // Create a list of nodes dynamically
+ List<ClusterNode> allNodes = IntStream.rangeClosed(1, numNodes)
+ .mapToObj(i -> (ClusterNode) new ClusterNodeImpl(
+ randomUUID(),
+ "node" + i,
+ new NetworkAddress("foo" + i, 1230 + i)))
+ .collect(Collectors.toList());
+
+ // Mock topology service behavior
+ for (var node : allNodes) {
+
when(topologyService.getByConsistentId(node.name())).thenReturn(node);
+ }
+ when(topologyService.allMembers()).thenReturn(allNodes);
+
+ when(messagingService.invoke(any(ClusterNode.class),
any(CmgInitMessage.class), anyLong()))
+ .thenReturn(initCompleteMessage());
+
+ // Initialize cluster
+ CompletableFuture<Void> initFuture = clusterInitializer.initCluster(
+ List.of(),
+ List.of(),
+ "cluster"
+ );
+
+ // Convert node names to a set for validation
+ var cmgNodeNameSet = allNodes.stream().map(ClusterNode::name).sorted()
Review Comment:
As usual, only limited `var` usages are allowed by the code style, so please
remove them
##########
modules/runner/src/main/java/org/apache/ignite/InitParametersBuilder.java:
##########
@@ -186,21 +173,13 @@ public InitParametersBuilder clusterName(String
clusterName) {
* @param clusterConfiguration Cluster configuration.
* @return {@code this} for chaining.
*/
- public InitParametersBuilder clusterConfiguration(String
clusterConfiguration) {
+ public InitParametersBuilder clusterConfiguration(@Nullable String
clusterConfiguration) {
Review Comment:
And why is this needed?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -107,29 +107,44 @@ public CompletableFuture<Void> initCluster(
String clusterName,
@Nullable String clusterConfiguration
) {
- if (metaStorageNodeNames.isEmpty()) {
- throw new IllegalArgumentException("Meta Storage node names list
must not be empty");
- }
-
if (metaStorageNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("Meta Storage node names must
not contain blank strings: " + metaStorageNodeNames);
}
- if (!cmgNodeNames.isEmpty() &&
cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
+ Set<String> msNodeNameSet =
metaStorageNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (msNodeNameSet.size() != metaStorageNodeNames.size()) {
+ throw new IllegalArgumentException("Meta Storage node names must
not contain duplicates: " + metaStorageNodeNames);
+ }
+
+ if (cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("CMG node names must not
contain blank strings: " + cmgNodeNames);
}
+ Set<String> cmgNodeNameSet =
cmgNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (cmgNodeNameSet.size() != cmgNodeNames.size()) {
+ throw new IllegalArgumentException("CMG node names must not
contain duplicates: " + metaStorageNodeNames);
+ }
+
+
+ if (msNodeNameSet.isEmpty() && cmgNodeNameSet.isEmpty()) {
+ var clusterNodes = clusterService.topologyService().allMembers();
+ var topologySize = clusterNodes.size();
+ var numberOfMsNodes = topologySize < 5 ? 3 : 5;
+ var chosenNodes =
clusterNodes.stream().map(ClusterNode::name).sorted().limit(numberOfMsNodes)
+ .collect(Collectors.toSet());
+ msNodeNameSet = chosenNodes;
+ cmgNodeNameSet = chosenNodes;
+ } else if (msNodeNameSet.isEmpty()) {
+ msNodeNameSet = cmgNodeNameSet;
+ } else if (cmgNodeNames.isEmpty()) {
+ cmgNodeNameSet = msNodeNameSet;
+ }
+
if (clusterName.isBlank()) {
Review Comment:
I would suggest to move this check to the very top, makes more sense to fail
early there
##########
modules/runner/src/main/java/org/apache/ignite/InitParametersBuilder.java:
##########
@@ -71,11 +70,9 @@ public InitParametersBuilder
metaStorageNodeNames(Collection<String> metaStorage
* @return {@code this} for chaining.
*/
public InitParametersBuilder metaStorageNodes(IgniteServer...
metaStorageNodes) {
- if (metaStorageNodes == null) {
- throw new IllegalArgumentException("Meta storage nodes cannot be
null.");
- }
- if (metaStorageNodes.length == 0) {
- throw new IllegalArgumentException("Meta storage nodes cannot be
empty.");
+ if (metaStorageNodes == null || metaStorageNodes.length == 0) {
Review Comment:
Why is this check needed? Why would you call `metaStorageNodes()` on its own
without parameters?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterInitializer.java:
##########
@@ -107,29 +107,44 @@ public CompletableFuture<Void> initCluster(
String clusterName,
@Nullable String clusterConfiguration
) {
- if (metaStorageNodeNames.isEmpty()) {
- throw new IllegalArgumentException("Meta Storage node names list
must not be empty");
- }
-
if (metaStorageNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("Meta Storage node names must
not contain blank strings: " + metaStorageNodeNames);
}
- if (!cmgNodeNames.isEmpty() &&
cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
+ Set<String> msNodeNameSet =
metaStorageNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (msNodeNameSet.size() != metaStorageNodeNames.size()) {
+ throw new IllegalArgumentException("Meta Storage node names must
not contain duplicates: " + metaStorageNodeNames);
+ }
+
+ if (cmgNodeNames.stream().anyMatch(StringUtils::nullOrBlank)) {
throw new IllegalArgumentException("CMG node names must not
contain blank strings: " + cmgNodeNames);
}
+ Set<String> cmgNodeNameSet =
cmgNodeNames.stream().map(String::trim).collect(toUnmodifiableSet());
+ if (cmgNodeNameSet.size() != cmgNodeNames.size()) {
+ throw new IllegalArgumentException("CMG node names must not
contain duplicates: " + metaStorageNodeNames);
+ }
+
+
+ if (msNodeNameSet.isEmpty() && cmgNodeNameSet.isEmpty()) {
+ var clusterNodes = clusterService.topologyService().allMembers();
+ var topologySize = clusterNodes.size();
+ var numberOfMsNodes = topologySize < 5 ? 3 : 5;
Review Comment:
Please add some comments here similar to what you have written in the docs.
These constants may confuse people who are reading the code.
--
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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
