PakhomovAlexander commented on code in PR #1267:
URL: https://github.com/apache/ignite-3/pull/1267#discussion_r1007949158
##########
modules/api/src/test/java/org/apache/ignite/network/NodeMetadataTest.java:
##########
@@ -15,8 +15,28 @@
* limitations under the License.
*/
-/**
- * Configuration schemas for Rest component.
- */
+package org.apache.ignite.network;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.nio.ByteBuffer;
+import org.junit.jupiter.api.Test;
+
+class NodeMetadataTest {
+
+ @Test
+ void testToByteBufferAndFromByteBuffer() {
Review Comment:
that is not required to write 'test' prefix
##########
modules/api/src/test/java/org/apache/ignite/network/NodeMetadataTest.java:
##########
@@ -15,8 +15,28 @@
* limitations under the License.
*/
-/**
- * Configuration schemas for Rest component.
- */
+package org.apache.ignite.network;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.nio.ByteBuffer;
+import org.junit.jupiter.api.Test;
+
+class NodeMetadataTest {
+
+ @Test
+ void testToByteBufferAndFromByteBuffer() {
+ NodeMetadata metadata = new NodeMetadata(10000);
+ ByteBuffer buffer = metadata.toByteBuffer();
+ NodeMetadata fromByteBuffer = NodeMetadata.fromByteBuffer(buffer);
+ assertEquals(metadata, fromByteBuffer);
+ }
-package org.apache.ignite.internal.rest.configuration;
+ @Test
+ void testFromByteBufferWithWrongContent() {
Review Comment:
It would be nice to add more test cases like the wrong version is set.
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java:
##########
@@ -51,12 +54,15 @@
import org.apache.ignite.network.NetworkAddress;
import org.apache.ignite.network.NodeFinder;
import org.apache.ignite.network.NodeFinderFactory;
+import org.apache.ignite.network.NodeMetadata;
/**
* Cluster service factory that uses ScaleCube for messaging and topology
services.
*/
public class ScaleCubeClusterServiceFactory {
- /** Logger. */
+ /**
+ * Logger.
Review Comment:
Unnecessary change.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli;
+
+import jakarta.inject.Singleton;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import
org.apache.ignite.internal.cli.call.cluster.topology.PhysicalTopologyCall;
+import org.apache.ignite.internal.cli.core.call.UrlCallInput;
+import org.apache.ignite.rest.client.model.ClusterNode;
+
+/**
+ * Registry to get a node URL by a node name.
+ */
+@Singleton
+public class NodeNameRegistry {
+ private final Map<String, String> nodeNameToNodeUrl = new
ConcurrentHashMap<>();
+ private volatile ScheduledExecutorService executor;
+
+ /**
+ * Start pulling updates from a node.
+ *
+ * @param nodeUrl Node URL.
+ */
+ public void startPullingUpdates(String nodeUrl) {
+ if (executor != null) {
+ synchronized (NodeNameRegistry.class) {
+ if (executor != null) {
+ executor.shutdown();
+ }
+ executor = Executors.newScheduledThreadPool(1);
+ executor.scheduleWithFixedDelay(() ->
updateNodeNames(nodeUrl), 0, 30, TimeUnit.SECONDS);
+ }
+ }
+ }
+
+ /**
+ * Stops pulling updates.
+ */
+ public void stopPullingUpdates() {
+ executor.shutdown();
+ executor = null;
+ }
+
+ public String getNodeUrl(String nodeName) {
+ return nodeNameToNodeUrl.get(nodeName);
+ }
+
+ public List<String> getAllNames() {
+ return new ArrayList<>(nodeNameToNodeUrl.keySet());
+ }
+
+ private void updateNodeNames(String nodeUrl) {
+ PhysicalTopologyCall topologyCall = new PhysicalTopologyCall();
+ topologyCall.execute(new UrlCallInput(nodeUrl)).body()
+ .forEach(it -> {
+ nodeNameToNodeUrl.put(it.getName(),
urlFromClusterNode(it));
+ });
+ }
+
+ private static String urlFromClusterNode(ClusterNode node) {
+ Objects.requireNonNull(node, "node must not be null");
+ return node.getAddress().getHost() + ":" +
node.getMetadata().getRestPort();
Review Comment:
It might be different: the TCP node address and the REST server address. So
I think it might be useful to spread not only the port but the whole address
through node metadata.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java:
##########
@@ -0,0 +1,86 @@
+/*
+ * 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.cli;
+
+import jakarta.inject.Singleton;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.TimeUnit;
+import
org.apache.ignite.internal.cli.call.cluster.topology.PhysicalTopologyCall;
+import org.apache.ignite.internal.cli.core.call.UrlCallInput;
+import org.apache.ignite.rest.client.model.ClusterNode;
+
+/**
+ * Registry to get a node URL by a node name.
+ */
+@Singleton
+public class NodeNameRegistry {
+ private final Map<String, String> nodeNameToNodeUrl = new
ConcurrentHashMap<>();
+ private volatile ScheduledExecutorService executor;
+
+ /**
+ * Start pulling updates from a node.
+ *
+ * @param nodeUrl Node URL.
+ */
+ public void startPullingUpdates(String nodeUrl) {
+ if (executor != null) {
+ synchronized (NodeNameRegistry.class) {
+ if (executor != null) {
+ executor.shutdown();
+ }
+ executor = Executors.newScheduledThreadPool(1);
+ executor.scheduleWithFixedDelay(() ->
updateNodeNames(nodeUrl), 0, 30, TimeUnit.SECONDS);
+ }
+ }
+ }
+
+ /**
+ * Stops pulling updates.
+ */
+ public void stopPullingUpdates() {
+ executor.shutdown();
+ executor = null;
+ }
+
+ public String getNodeUrl(String nodeName) {
+ return nodeNameToNodeUrl.get(nodeName);
+ }
+
+ public List<String> getAllNames() {
+ return new ArrayList<>(nodeNameToNodeUrl.keySet());
+ }
+
+ private void updateNodeNames(String nodeUrl) {
+ PhysicalTopologyCall topologyCall = new PhysicalTopologyCall();
+ topologyCall.execute(new UrlCallInput(nodeUrl)).body()
+ .forEach(it -> {
+ nodeNameToNodeUrl.put(it.getName(),
urlFromClusterNode(it));
Review Comment:
`put` replaces existing values but outdated ones won't be removed.
##########
modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java:
##########
@@ -131,7 +138,7 @@ public void onMembershipEvent(MembershipEvent event) {
topologyService.onMembershipEvent(event);
}
})
- .config(opts -> opts.memberAlias(consistentId))
+ .config(opts ->
opts.memberAlias(consistentId).metadata(new
NodeMetadata(restConfiguration.port().value())))
Review Comment:
No, the rest configuration is taken from a file but there is a `portRange`
config that could be used if the original rest.port is not available. So the
REST server can be started on another port, look at `RestComponent.start()`.
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/Session.java:
##########
@@ -65,4 +65,8 @@ public String jdbcUrl() {
public void setJdbcUrl(String jdbcUrl) {
this.jdbcUrl = jdbcUrl;
}
+
+ public void onConnect() {
Review Comment:
Empty method
##########
modules/api/src/test/java/org/apache/ignite/network/NodeMetadataTest.java:
##########
@@ -15,8 +15,28 @@
* limitations under the License.
*/
-/**
- * Configuration schemas for Rest component.
- */
+package org.apache.ignite.network;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+
+import java.nio.ByteBuffer;
+import org.junit.jupiter.api.Test;
+
+class NodeMetadataTest {
+
+ @Test
+ void testToByteBufferAndFromByteBuffer() {
+ NodeMetadata metadata = new NodeMetadata(10000);
+ ByteBuffer buffer = metadata.toByteBuffer();
+ NodeMetadata fromByteBuffer = NodeMetadata.fromByteBuffer(buffer);
+ assertEquals(metadata, fromByteBuffer);
+ }
-package org.apache.ignite.internal.rest.configuration;
+ @Test
+ void testFromByteBufferWithWrongContent() {
Review Comment:
And why did you decide to return null but not to throw exception?
--
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]
