sashapolo commented on code in PR #1267: URL: https://github.com/apache/ignite-3/pull/1267#discussion_r1017702888
########## modules/api/src/main/java/org/apache/ignite/network/NodeMetadata.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.network; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonGetter; +import com.fasterxml.jackson.annotation.JsonProperty; +import java.io.Serializable; +import java.util.Objects; + +/** + * Contains metadata of the cluster node. + */ +public class NodeMetadata implements Serializable { Review Comment: Why is this class both `Serializable` and is annotated with Jackson? I'm also not sure that we also want to add Jackson dependency to the API module ########## modules/api/src/main/java/org/apache/ignite/network/NodeMetadata.java: ########## @@ -0,0 +1,71 @@ +/* + * 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.network; + +import com.fasterxml.jackson.annotation.JsonCreator; +import com.fasterxml.jackson.annotation.JsonGetter; +import com.fasterxml.jackson.annotation.JsonProperty; +import java.io.Serializable; +import java.util.Objects; + +/** + * Contains metadata of the cluster node. + */ +public class NodeMetadata implements Serializable { + private static final long serialVersionUID = 3216463261002854096L; + + private String restHost; + + private int restPort; + + public NodeMetadata() { Review Comment: Why do you need this constructor? ########## modules/api/src/main/java/org/apache/ignite/network/ClusterNode.java: ########## @@ -33,6 +33,23 @@ public class ClusterNode implements Serializable { /** Network address of this node. */ private final NetworkAddress address; + private final NodeMetadata nodeMetadata; Review Comment: Please annotate this field and its getter as `Nullable` ########## modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/Await.java: ########## @@ -0,0 +1,49 @@ +/* + * 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 java.time.Duration; +import java.util.function.Supplier; + +/** Utility class to await some condition. */ +public final class Await { + + private Await() { + } + + /** Awaits a condition. */ + public static void await(Supplier<Boolean> supplier, Duration timeout) { Review Comment: There's a `IgniteTestUtils#waitForCondition` method that does the same thing ########## modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/NodeNameTest.java: ########## @@ -0,0 +1,96 @@ +/* + * 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.commands; + +import static org.junit.jupiter.api.Assertions.assertAll; + +import jakarta.inject.Inject; +import java.time.Duration; +import java.util.Optional; +import org.apache.ignite.internal.cli.Await; +import org.apache.ignite.internal.cli.NodeNameRegistry; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** Tests for ignite node commands with a provided node name. */ +public class NodeNameTest extends CliCommandTestNotInitializedIntegrationBase { + + @Inject + private NodeNameRegistry nodeNameRegistry; Review Comment: There's a similar field in the `CliCommandTestNotInitializedIntegrationBase` class, do you really need this one as well? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; + + /** + * Start pulling updates from a node. + * + * @param nodeUrl Node URL. + */ + public void startPullingUpdates(String nodeUrl) { + stopPullingUpdates(); + synchronized (this) { Review Comment: I think it would be shorter to simply mark the whole method as `synchronized` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; Review Comment: Why is this field `volatile`? You always access it under the `synchronized` block ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; + + /** + * Start pulling updates from a node. + * + * @param nodeUrl Node URL. + */ + public void startPullingUpdates(String nodeUrl) { + stopPullingUpdates(); + synchronized (this) { + if (executor == null) { + executor = Executors.newScheduledThreadPool(1); Review Comment: 1. You can use `newSingleThreadScheduledExecutor` 2. Please provide a ThreadFactory which will provide meaningful thread names (see similar places in other components) ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); Review Comment: Looks like you don't need a `ConcurrentHashMap` here, it's always accessed under the `readWriteLock` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; + + /** + * Start pulling updates from a node. + * + * @param nodeUrl Node URL. + */ + public void startPullingUpdates(String nodeUrl) { + stopPullingUpdates(); + synchronized (this) { + if (executor == null) { + executor = Executors.newScheduledThreadPool(1); + executor.scheduleWithFixedDelay(() -> updateNodeNames(nodeUrl), 0, 5, TimeUnit.SECONDS); + } + } + } + + /** + * Stops pulling updates. + */ + public synchronized void stopPullingUpdates() { + if (executor != null) { + executor.shutdown(); + executor = null; + } + } + + /** + * Gets a node url by a provided node name. + */ + public Optional<String> getNodeUrl(String nodeName) { + readWriteLock.readLock().lock(); + try { + return Optional.ofNullable(nodeNameToNodeUrl.get(nodeName)); + } finally { + readWriteLock.readLock().unlock(); + } + } + + /** + * Gets all node names. + */ + public List<String> getAllNames() { + readWriteLock.readLock().lock(); + try { + return new ArrayList<>(nodeNameToNodeUrl.keySet()); + } finally { + readWriteLock.readLock().unlock(); + } + } + + private void updateNodeNames(String nodeUrl) { + readWriteLock.writeLock().lock(); + try { + nodeNameToNodeUrl.clear(); + PhysicalTopologyCall topologyCall = new PhysicalTopologyCall(); + topologyCall.execute(new UrlCallInput(nodeUrl)).body() + .forEach(it -> { + nodeNameToNodeUrl.put(it.getName(), urlFromClusterNode(it.getMetadata())); Review Comment: This may be a bug: `urlFromClusterNode` may return `null`, while `ConcurrentHashMap` does not allow null values ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/OptionsConstants.java: ########## @@ -38,6 +38,9 @@ public class OptionsConstants { /** Node URL option description. */ public static final String NODE_URL_DESC = "URL of ignite node"; + /** Node URL or name option description. */ + public static final String NODE_URL_OR_NAME_DESC = "URL or name of ignite node"; Review Comment: ```suggestion public static final String NODE_URL_OR_NAME_DESC = "URL or name of an Ignite node"; ``` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/node/NodeUrl.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.commands.node; + +import java.net.URL; + +/** Node URL. */ +public class NodeUrl { Review Comment: What's the purpose of this class? Can we simply use `URL` instead? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/node/NodeUrl.java: ########## @@ -0,0 +1,33 @@ +/* + * 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.commands.node; + +import java.net.URL; + +/** Node URL. */ +public class NodeUrl { + private URL nodeUrl; Review Comment: ```suggestion private final URL nodeUrl; ``` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/converters/NodeNameOrUrlConverter.java: ########## @@ -0,0 +1,62 @@ +/* + * 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.core.converters; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.regex.Pattern; +import org.apache.ignite.internal.cli.NodeNameRegistry; +import org.apache.ignite.internal.cli.commands.node.NodeUrl; +import picocli.CommandLine; +import picocli.CommandLine.TypeConversionException; + +/** Converter for {@link NodeUrl}. */ +public class NodeNameOrUrlConverter implements CommandLine.ITypeConverter<NodeUrl> { + + private static final Pattern URL_PATTERN = Pattern.compile("^.*[/:].*"); Review Comment: So we distinguish a URL from a name by looking for a semicolon and slashes? Maybe we can remove this logic altogether and simply try to always parse a URL and fallback to node name lookup if it fails? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/node/NodeUrlMixin.java: ########## @@ -17,23 +17,63 @@ package org.apache.ignite.internal.cli.commands.node; +import static org.apache.ignite.internal.cli.commands.OptionsConstants.NODE_NAME_DESC; +import static org.apache.ignite.internal.cli.commands.OptionsConstants.NODE_NAME_OPTION; +import static org.apache.ignite.internal.cli.commands.OptionsConstants.NODE_NAME_OPTION_SHORT; import static org.apache.ignite.internal.cli.commands.OptionsConstants.NODE_URL_DESC; import static org.apache.ignite.internal.cli.commands.OptionsConstants.NODE_URL_OPTION; import static org.apache.ignite.internal.cli.commands.OptionsConstants.URL_OPTION_SHORT; +import jakarta.inject.Inject; import java.net.URL; +import org.apache.ignite.internal.cli.NodeNameRegistry; import org.apache.ignite.internal.cli.core.converters.UrlConverter; +import org.apache.ignite.internal.cli.deprecated.IgniteCliException; +import picocli.CommandLine.ArgGroup; import picocli.CommandLine.Option; /** * Mixin class for node URL option. */ public class NodeUrlMixin { - /** Node URL option. */ - @Option(names = {URL_OPTION_SHORT, NODE_URL_OPTION}, description = NODE_URL_DESC, converter = UrlConverter.class) - private URL nodeUrl; + @ArgGroup + private Options options; + + @Inject + NodeNameRegistry nodeNameRegistry; + + private static class Options { + + /** + * Node URL option. + */ + @Option(names = {URL_OPTION_SHORT, NODE_URL_OPTION}, description = NODE_URL_DESC, converter = UrlConverter.class) + private URL nodeUrl; + + /** + * Node name option. + */ + @Option(names = {NODE_NAME_OPTION_SHORT, NODE_NAME_OPTION}, description = NODE_NAME_DESC) + private String nodeName; + } + + /** + * Returns node URL. + * + * @return Node URL + */ public String getNodeUrl() { - return nodeUrl != null ? nodeUrl.toString() : null; + if (options == null) { + return null; + } else { + if (options.nodeUrl != null) { + return options.nodeUrl.toString(); + } else { + return nodeNameRegistry.getNodeUrl(options.nodeName) + .orElseThrow(() -> new IgniteCliException("Node " + options.nodeName + + " not found. Provide valid name or use URL")); Review Comment: ```suggestion + " not found. Provide a valid name or use a URL")); ``` ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/converters/NodeNameOrUrlConverter.java: ########## @@ -0,0 +1,62 @@ +/* + * 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.core.converters; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.regex.Pattern; +import org.apache.ignite.internal.cli.NodeNameRegistry; +import org.apache.ignite.internal.cli.commands.node.NodeUrl; +import picocli.CommandLine; +import picocli.CommandLine.TypeConversionException; + +/** Converter for {@link NodeUrl}. */ +public class NodeNameOrUrlConverter implements CommandLine.ITypeConverter<NodeUrl> { + + private static final Pattern URL_PATTERN = Pattern.compile("^.*[/:].*"); + + private final NodeNameRegistry nodeNameRegistry; + + public NodeNameOrUrlConverter(NodeNameRegistry nodeNameRegistry) { + this.nodeNameRegistry = nodeNameRegistry; + } + + private static URL stringToUrl(String str) { + try { + return new URL(str); + } catch (MalformedURLException e) { + throw new TypeConversionException("Invalid URL '" + str + "' (" + e.getMessage() + ")"); + } + } + + @Override + public NodeUrl convert(String input) throws Exception { + boolean isUrl = URL_PATTERN.matcher(input).find(); Review Comment: I think `matches()` is a more suitable call here ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/NodeNameDynamicCompleter.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.core.repl.completer; + +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.ignite.internal.cli.NodeNameRegistry; + +/** + * Completes typed words with node names. + */ +public class NodeNameDynamicCompleter implements DynamicCompleter { + + /** Words, after those the completer should have been activated. */ + private final Set<String> activationPostfixes; + + /** Node names registry. */ + private final NodeNameRegistry nodeNameRegistry; + + /** Default constructor that creates an instance from a given set of postfixes that trigger the completion. */ + public NodeNameDynamicCompleter(Set<String> activationPostfixes, NodeNameRegistry nodeNameRegistry) { + this.activationPostfixes = activationPostfixes; + this.nodeNameRegistry = nodeNameRegistry; + } + + @Override + public List<String> complete(String[] words) { + String lastWord = beforeLastNotEmptyWord(0, words); + String beforeLastWord = beforeLastNotEmptyWord(1, words); + if (activationPostfixes.contains(lastWord)) { + return nodeNameRegistry.getAllNames(); + } else if (activationPostfixes.contains(beforeLastWord)) { + return nodeNameRegistry.getAllNames().stream() + .filter(it -> it.startsWith(lastWord)) + .collect(Collectors.toList()); + } else { + return Collections.emptyList(); + } + } + + private static String beforeLastNotEmptyWord(int index, String[] words) { + for (int i = words.length - 1 - index; i >= 0; i--) { + if (!words[i].isEmpty()) { Review Comment: Shall we use `isBlank` instead of `isEmpty`? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/repl/completer/NodeNameDynamicCompleter.java: ########## @@ -0,0 +1,66 @@ +/* + * 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.core.repl.completer; + +import java.util.Collections; +import java.util.List; +import java.util.Set; +import java.util.stream.Collectors; +import org.apache.ignite.internal.cli.NodeNameRegistry; + +/** + * Completes typed words with node names. + */ +public class NodeNameDynamicCompleter implements DynamicCompleter { + + /** Words, after those the completer should have been activated. */ + private final Set<String> activationPostfixes; + + /** Node names registry. */ + private final NodeNameRegistry nodeNameRegistry; + + /** Default constructor that creates an instance from a given set of postfixes that trigger the completion. */ + public NodeNameDynamicCompleter(Set<String> activationPostfixes, NodeNameRegistry nodeNameRegistry) { + this.activationPostfixes = activationPostfixes; + this.nodeNameRegistry = nodeNameRegistry; + } + + @Override + public List<String> complete(String[] words) { + String lastWord = beforeLastNotEmptyWord(0, words); + String beforeLastWord = beforeLastNotEmptyWord(1, words); + if (activationPostfixes.contains(lastWord)) { + return nodeNameRegistry.getAllNames(); + } else if (activationPostfixes.contains(beforeLastWord)) { + return nodeNameRegistry.getAllNames().stream() + .filter(it -> it.startsWith(lastWord)) + .collect(Collectors.toList()); + } else { + return Collections.emptyList(); + } + } + + private static String beforeLastNotEmptyWord(int index, String[] words) { + for (int i = words.length - 1 - index; i >= 0; i--) { Review Comment: This method does not check if `index` is actually a valid index inside the `words` array, is that ok? ########## modules/network-api/src/main/java/org/apache/ignite/network/ClusterService.java: ########## @@ -56,5 +56,7 @@ default void stop() { * * @return {@code true} if cluster service is stopped, {@code false} otherwise. */ - public boolean isStopped(); + boolean isStopped(); + + void updateMetadata(NodeMetadata metadata); Review Comment: Please add a javadoc (I wonder if you can even pass the CI checks without it) ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/util/ArrayUtils.java: ########## @@ -0,0 +1,35 @@ +/* + * 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.util; + +/** Utility class for arrays. */ +public final class ArrayUtils { + + private ArrayUtils() { + } + + /** Finds the last not empty word in array. */ + public static String findLastNotEmptyWord(String[] words) { + for (int i = words.length - 1; i >= 0; i--) { Review Comment: Same thing about boundaries checks and `isBlank` ########## modules/network/src/main/java/org/apache/ignite/network/scalecube/NodeMetadataCodec.java: ########## @@ -0,0 +1,72 @@ +/* + * 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.network.scalecube; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; +import io.scalecube.cluster.metadata.MetadataCodec; +import java.nio.ByteBuffer; +import org.apache.ignite.network.NodeMetadata; + +/** + * Codec for serialization and deserialization of {@link NodeMetadata}. + */ +public class NodeMetadataCodec implements MetadataCodec { Review Comment: Why do you need this class? The default `MetadataCodec` in Scalecube is `JdkMetadataCodec` which is enough for our purposes, since `NodeMetadata` is `Serializable`. I think we don't need this class and Jackson dependency as well ########## modules/cli/src/integrationTest/java/org/apache/ignite/internal/cli/commands/NodeNameTest.java: ########## @@ -0,0 +1,97 @@ +/* + * 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.commands; + +import static org.junit.jupiter.api.Assertions.assertAll; + +import jakarta.inject.Inject; +import java.time.Duration; +import java.util.Optional; +import org.apache.ignite.internal.cli.Await; +import org.apache.ignite.internal.cli.NodeNameRegistry; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; + +/** Tests for ignite node commands with a provided node name. */ +public class NodeNameTest extends CliCommandTestNotInitializedIntegrationBase { + + @Inject + NodeNameRegistry nodeNameRegistry; + private String nodeName; + + @BeforeEach + void setUp() { + nodeNameRegistry.startPullingUpdates("http://localhost:10301";); + // wait to pulling node names + Await.await(() -> !nodeNameRegistry.getAllNames().isEmpty(), Duration.ofSeconds(5)); + Optional<String> nodeName = nodeNameRegistry.getAllNames().stream().findFirst(); + Assertions.assertTrue(nodeName.isPresent()); Review Comment: Why do you even need an assertion here? `get` will throw an exception if the Optional is empty ########## modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java: ########## @@ -190,6 +201,17 @@ public void beforeNodeStop() { public boolean isStopped() { return shutdownFuture.isDone(); } + + @Override + public void updateMetadata(NodeMetadata metadata) { + cluster.updateMetadata(metadata).subscribeOn(scheduler).subscribe(); + MembershipEvent membershipEvent = createUpdated(cluster.member(), + cluster.<NodeMetadata>metadata().map(METADATA_CODEC::serialize).orElse(null), + METADATA_CODEC.serialize(metadata), + System.currentTimeMillis() + ); + topologyService.onMembershipEvent(membershipEvent); Review Comment: Why do we need to emit an event here? I can see that `updateMetadata` calls `updateIncarnation`, which will propagate this information to the remote peers ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; + + /** + * Start pulling updates from a node. + * + * @param nodeUrl Node URL. + */ + public void startPullingUpdates(String nodeUrl) { + stopPullingUpdates(); + synchronized (this) { + if (executor == null) { + executor = Executors.newScheduledThreadPool(1); + executor.scheduleWithFixedDelay(() -> updateNodeNames(nodeUrl), 0, 5, TimeUnit.SECONDS); + } + } + } + + /** + * Stops pulling updates. + */ + public synchronized void stopPullingUpdates() { + if (executor != null) { + executor.shutdown(); + executor = null; + } + } + + /** + * Gets a node url by a provided node name. + */ + public Optional<String> getNodeUrl(String nodeName) { + readWriteLock.readLock().lock(); + try { + return Optional.ofNullable(nodeNameToNodeUrl.get(nodeName)); + } finally { + readWriteLock.readLock().unlock(); + } + } + + /** + * Gets all node names. + */ + public List<String> getAllNames() { + readWriteLock.readLock().lock(); + try { + return new ArrayList<>(nodeNameToNodeUrl.keySet()); + } finally { + readWriteLock.readLock().unlock(); + } + } + + private void updateNodeNames(String nodeUrl) { + readWriteLock.writeLock().lock(); Review Comment: Nice suggestion, totally agree. You don't even need any CAS here, simple volatile field will be sufficient ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/NodeNameRegistry.java: ########## @@ -0,0 +1,113 @@ +/* + * 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.Optional; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.Executors; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.locks.ReadWriteLock; +import java.util.concurrent.locks.ReentrantReadWriteLock; +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.NodeMetadata; + +/** + * Registry to get a node URL by a node name. + */ +@Singleton +public class NodeNameRegistry { + private final Map<String, String> nodeNameToNodeUrl = new ConcurrentHashMap<>(); + private final ReadWriteLock readWriteLock = new ReentrantReadWriteLock(); + private volatile ScheduledExecutorService executor; + + /** + * Start pulling updates from a node. + * + * @param nodeUrl Node URL. + */ + public void startPullingUpdates(String nodeUrl) { + stopPullingUpdates(); + synchronized (this) { + if (executor == null) { + executor = Executors.newScheduledThreadPool(1); + executor.scheduleWithFixedDelay(() -> updateNodeNames(nodeUrl), 0, 5, TimeUnit.SECONDS); + } + } + } + + /** + * Stops pulling updates. + */ + public synchronized void stopPullingUpdates() { + if (executor != null) { + executor.shutdown(); Review Comment: I would recommend using `IgniteUtils.shutdownAndAwaitTermination` ########## modules/network/src/main/java/org/apache/ignite/network/scalecube/ScaleCubeClusterServiceFactory.java: ########## @@ -59,6 +66,11 @@ public class ScaleCubeClusterServiceFactory { /** Logger. */ private static final IgniteLogger LOG = Loggers.forClass(ScaleCubeClusterServiceFactory.class); + /** Metadata codec. */ + private static final NodeMetadataCodec METADATA_CODEC = NodeMetadataCodec.INSTANCE; + /** Scalecube cluster executor to update metadata. */ + private final Scheduler scheduler = Schedulers.fromExecutor(Executors.newSingleThreadExecutor()); Review Comment: Why do you need this executor? ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/OptionsConstants.java: ########## @@ -38,6 +38,9 @@ public class OptionsConstants { /** Node URL option description. */ public static final String NODE_URL_DESC = "URL of ignite node"; + /** Node URL or name option description. */ + public static final String NODE_URL_OR_NAME_DESC = "URL or name of ignite node"; Review Comment: Please fix other descriptions accordingly ########## modules/cli/src/main/java/org/apache/ignite/internal/cli/core/converters/NodeNameOrUrlConverter.java: ########## @@ -0,0 +1,62 @@ +/* + * 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.core.converters; + +import java.net.MalformedURLException; +import java.net.URL; +import java.util.regex.Pattern; +import org.apache.ignite.internal.cli.NodeNameRegistry; +import org.apache.ignite.internal.cli.commands.node.NodeUrl; +import picocli.CommandLine; +import picocli.CommandLine.TypeConversionException; + +/** Converter for {@link NodeUrl}. */ +public class NodeNameOrUrlConverter implements CommandLine.ITypeConverter<NodeUrl> { + + private static final Pattern URL_PATTERN = Pattern.compile("^.*[/:].*"); + + private final NodeNameRegistry nodeNameRegistry; + + public NodeNameOrUrlConverter(NodeNameRegistry nodeNameRegistry) { + this.nodeNameRegistry = nodeNameRegistry; + } + + private static URL stringToUrl(String str) { + try { + return new URL(str); + } catch (MalformedURLException e) { + throw new TypeConversionException("Invalid URL '" + str + "' (" + e.getMessage() + ")"); + } + } + + @Override + public NodeUrl convert(String input) throws Exception { + boolean isUrl = URL_PATTERN.matcher(input).find(); + if (isUrl) { + return new NodeUrl(stringToUrl(input)); + } else { + return new NodeUrl(findNodeUrlByNodeName(input)); + } + } + + private URL findNodeUrlByNodeName(String name) { + return nodeNameRegistry.getNodeUrl(name) + .map(NodeNameOrUrlConverter::stringToUrl) Review Comment: Why do we store URL as Strings inside the `nodeNameRegistry`? I think we should store URLs there, this way we will avoid unnecessary conversions -- 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]
