sanpwc commented on code in PR #1819:
URL: https://github.com/apache/ignite-3/pull/1819#discussion_r1144378368
##########
examples/config/ignite-config.conf:
##########
@@ -1,11 +1,12 @@
-{
- network: {
- port: 3344,
- portRange: 10,
- nodeFinder: {
- netClusterNodes: [
- "localhost:3344"
- ]
- }
+network {
+ nodeFinder {
+ netClusterNodes=[
+ "localhost:3344"
+ ]
}
+ port=3344
+ portRange=10
+}
+nodeAttributes {
+ nodeAttributes="{nodeName:'node1',region:'EU',storage:'SSD',dataRegion:10}"
Review Comment:
nodeName is a mandatory one? If true - that's not a very good approach.
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/ItClusterManagerTest.java:
##########
@@ -412,10 +413,12 @@ void
nonCmgNodeAddedLaterGetsLogicalTopologyChanges(TestInfo testInfo) throws Ex
assertTrue(waitForCondition(() ->
nonCmgTopology.getLogicalTopology().nodes().size() == 2, 10_000));
}
+ // TODO:???
private ClusterNode[] currentPhysicalTopology() {
return cluster.stream()
.map(MockNode::localMember)
- .toArray(ClusterNode[]::new);
+ .map(n -> new LogicalNode(n, ""))
+ .toArray(LogicalNode[]::new);
Review Comment:
Why do we return LogicalNode from current**Physical**Topology?
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNode.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.internal.cluster.management.topology.api;
+
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.network.ClusterNode;
+import org.apache.ignite.network.NetworkAddress;
+
+/**
+ * Representation of a logical node in a cluster.
+ */
+public class LogicalNode extends ClusterNode {
+ /** Node's attributes. */
+ private final String nodeAttributes;
+
+ /**
+ * Constructor.
+ *
+ * @param id Local id that changes between restarts.
+ * @param name Unique name of a member in a cluster.
+ * @param address Node address.
+ */
+ public LogicalNode(String id, String name, NetworkAddress address, String
nodeAttributes) {
Review Comment:
Formatting. More than three attributes => one per line.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributesConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.cluster.management.configuration;
+
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import org.apache.ignite.configuration.annotation.Value;
+
+/**
+ * Node's attributes configuration schema.
+ */
+@ConfigurationRoot(rootName = "nodeAttributes", type = ConfigurationType.LOCAL)
+public class NodeAttributesConfigurationSchema {
+ @Value(hasDefault = true)
+ public final String nodeAttributes = "";
Review Comment:
Should we use named list (@NamedConfigValue) instead? Named list is actually
a map, and in our case name is property name, e.g. 'region', 'storage' and
value is just a String. Such approach will ease the access pattern for the kv
attributes. We also should limit the size of node attributes.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributesConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.cluster.management.configuration;
+
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import org.apache.ignite.configuration.annotation.Value;
+
+/**
+ * Node's attributes configuration schema.
+ */
+@ConfigurationRoot(rootName = "nodeAttributes", type = ConfigurationType.LOCAL)
+public class NodeAttributesConfigurationSchema {
+ @Value(hasDefault = true)
Review Comment:
As was mentioned above, I don't think that it should have a default, it's an
optional attribute.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/ClusterManagementConfigurationModule.java:
##########
@@ -36,6 +36,6 @@ public ConfigurationType type() {
@Override
public Collection<RootKey<?, ?>> rootKeys() {
- return List.of(ClusterManagementConfiguration.KEY);
+ return List.of(ClusterManagementConfiguration.KEY,
NodeAttributesConfiguration.KEY);
Review Comment:
Why it's in ClusterManagementConfigurationModule? You've declared
NodeAttributesConfigurationSchema as @ConfigurationRoot. E.g.
NetworkConfiguration has it's own module - NetworkConfigurationModule.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -135,8 +135,8 @@ public CompletableFuture<Void>
updateClusterState(ClusterState clusterState) {
* otherwise.
* @see ValidationManager
*/
- public CompletableFuture<Void> startJoinCluster(ClusterTag clusterTag) {
- ClusterNodeMessage localNodeMessage =
nodeMessage(clusterService.topologyService().localMember());
+ public CompletableFuture<Void> startJoinCluster(ClusterTag clusterTag,
String nodeAttributes) {
Review Comment:
Please check the aforementioned comments: it's a bit confusing to have
nodeAttributes both in start and complete.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/LogicalTopologyImpl.java:
##########
@@ -67,25 +68,25 @@ private LogicalTopologySnapshot readLogicalTopology() {
}
@Override
- public void onNodeValidated(ClusterNode node) {
+ public void onNodeValidated(LogicalNode node) {
notifyListeners(listener -> listener.onNodeValidated(node),
"onNodeValidated");
}
@Override
- public void onNodeInvalidated(ClusterNode node) {
+ public void onNodeInvalidated(LogicalNode node) {
notifyListeners(listener -> listener.onNodeInvalidated(node),
"onNodeInvalidated");
}
@Override
- public void putNode(ClusterNode nodeToPut) {
+ public void putNode(LogicalNode nodeToPut) {
LogicalTopologySnapshot snapshot = readLogicalTopology();
- Map<String, ClusterNode> mapByName = snapshot.nodes().stream()
+ Map<String, LogicalNode> mapByName = snapshot.nodes().stream()
.collect(toMap(ClusterNode::name, identity()));
Review Comment:
There are still some mentioned of ClusterNode (not only this one) that might
(and probably should be changed to LogicalNode) ClusterNode::name ->
LogicalNode::name etc.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/raft/CmgRaftService.java:
##########
@@ -179,6 +179,10 @@ public CompletableFuture<Void> completeJoinCluster() {
});
}
+ public CompletableFuture<Void> completeJoinCluster() {
Review Comment:
What's that? First of all, as was mentioned above nodeAttributes should be
optional. However, in any case, why we need completeJoinCluster that just
return completed future?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/util/DistributionZonesTestUtil.java:
##########
@@ -172,6 +173,22 @@ public static void assertLogicalTopology(
assertTrue(waitForCondition(() ->
Arrays.equals(keyValueStorage.get(zonesLogicalTopologyKey().bytes()).value(),
nodes), 1000));
}
+ /**
+ * Asasas.
Review Comment:
azaza?
##########
modules/distribution-zones/src/test/java/org/apache/ignite/internal/distributionzones/util/DistributionZonesTestUtil.java:
##########
@@ -172,6 +173,22 @@ public static void assertLogicalTopology(
assertTrue(waitForCondition(() ->
Arrays.equals(keyValueStorage.get(zonesLogicalTopologyKey().bytes()).value(),
nodes), 1000));
}
+ /**
+ * Asasas.
+ *
+ * @param clusterNodes asdsa.
+ * @param keyValueStorage asda.
+ * @throws InterruptedException asda.
+ */
+ public static void assertLogicalTopologyWithNodeNames(
Review Comment:
Formatting, two params, one line.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/ClusterManagementGroupManager.java:
##########
@@ -542,15 +549,15 @@ private void handleClusterState(ClusterStateMessage msg,
String senderConsistent
private CompletableFuture<CmgRaftService>
completeJoinIfTryingToRejoin(CmgRaftService cmgRaftService) {
if (attemptedCompleteJoinOnStart) {
- return cmgRaftService.completeJoinCluster()
+ return
cmgRaftService.completeJoinCluster(nodeAttributes.nodeAttributes().value())
.thenApply(unused -> cmgRaftService);
} else {
return completedFuture(cmgRaftService);
}
}
private CompletableFuture<CmgRaftService> joinCluster(CmgRaftService
service, ClusterTag clusterTag) {
- return service.startJoinCluster(clusterTag)
+ return service.startJoinCluster(clusterTag,
nodeAttributes.nodeAttributes().value())
Review Comment:
Not sure, you will send nodeAttributes both wihtin startJoin and
completeJoin? Why?
##########
modules/cluster-management/src/integrationTest/java/org/apache/ignite/internal/cluster/management/raft/ItCmgRaftServiceTest.java:
##########
@@ -209,8 +210,8 @@ void testLogicalTopology() {
Node node1 = cluster.get(0);
Node node2 = cluster.get(1);
- ClusterNode clusterNode1 = node1.localMember();
- ClusterNode clusterNode2 = node2.localMember();
+ LogicalNode clusterNode1 = new LogicalNode(node1.localMember(), "");
Review Comment:
Node attributes is an optional parameter. So that we don't need `,""`.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributesConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.cluster.management.configuration;
+
+import org.apache.ignite.configuration.annotation.ConfigurationRoot;
+import org.apache.ignite.configuration.annotation.ConfigurationType;
+import org.apache.ignite.configuration.annotation.Value;
+
+/**
+ * Node's attributes configuration schema.
Review Comment:
It's not quire clear from the javadoc what is node's attributes, how to use
them. It's a part of the public API. So we should add more details here,
explaining zone filters, providing examples, etc.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/configuration/NodeAttributesConfigurationSchema.java:
##########
@@ -0,0 +1,31 @@
+/*
+ * 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.internal.cluster.management.configuration;
Review Comment:
Why it's internal? So, year user will provide local node cfg as plain string
or HOCON but scheme should be public otherwise the user won't know what to
specify.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNode.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.internal.cluster.management.topology.api;
+
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.network.ClusterNode;
+import org.apache.ignite.network.NetworkAddress;
+
+/**
+ * Representation of a logical node in a cluster.
+ */
+public class LogicalNode extends ClusterNode {
+ /** Node's attributes. */
+ private final String nodeAttributes;
Review Comment:
Well, from the general point of view, we should have KV map instead. If it's
required to have string representation for some purposes we can have both or
convert by demand it it's rather rare.
##########
modules/cluster-management/src/main/java/org/apache/ignite/internal/cluster/management/topology/api/LogicalNode.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * 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.internal.cluster.management.topology.api;
+
+import org.apache.ignite.internal.tostring.S;
+import org.apache.ignite.network.ClusterNode;
+import org.apache.ignite.network.NetworkAddress;
+
+/**
+ * Representation of a logical node in a cluster.
+ */
+public class LogicalNode extends ClusterNode {
+ /** Node's attributes. */
+ private final String nodeAttributes;
+
+ /**
+ * Constructor.
+ *
+ * @param id Local id that changes between restarts.
+ * @param name Unique name of a member in a cluster.
+ * @param address Node address.
+ */
+ public LogicalNode(String id, String name, NetworkAddress address, String
nodeAttributes) {
+ super(id, name, address);
+
+ this.nodeAttributes = nodeAttributes;
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param clusterNode Represents a node in a cluster..
Review Comment:
It's either "..." or '.')) And yep both clusterNode and LogicalNode
represents a node in a cluster. We should either remove meaningless javadoc or
add more definitions, e.g. saying that it's not a cluster, put network topology
etc.
##########
modules/runner/src/integrationTest/java/org/apache/ignite/internal/compute/ItLogicalTopologyTest.java:
##########
@@ -120,6 +137,46 @@ void receivesLogicalTopologyEvents() throws Exception {
assertThat(events, is(empty()));
}
+ @Test
+ void receivesLogicalTopologyEventsWithNodeAttributes() throws Exception {
Review Comment:
Do we have similar test for logicalTopologyOnLeader?
--
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]
