sashapolo commented on code in PR #4320:
URL: https://github.com/apache/ignite-3/pull/4320#discussion_r1741158133
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -17,13 +17,29 @@
package org.apache.ignite.internal.network;
+import static java.util.stream.Collectors.toList;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Arrays;
import java.util.List;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.network.NetworkAddress;
/**
- * {@code NodeFinder} implementation that encapsulates a predefined list of
network addresses.
+ * {@code NodeFinder} implementation that encapsulates a predefined list of
network addresses and/or host names.
+ *
+ * <p>IP addresses are returned as is (up to a format change).
+ *
+ * <p>Names are resolved. If a name is resolved to at least one addreess, all
these addresses will be returned. If it resolves
+ * to nothing, this name will not add anything to the output; no exception
will be thrown in such case.
+ *
+ * <p>If among addresses to which a name is resolved there is a loopback
address, only this address is contributed for this name.
Review Comment:
```suggestion
* <p>If a loopback address is among the resolved addresses, only this
address is contributed for this name.
```
##########
modules/network/src/test/java/org/apache/ignite/internal/network/StaticNodeFinderTest.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.network;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.InetAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.network.NetworkAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(WorkDirectoryExtension.class)
+class StaticNodeFinderTest {
+ @WorkDirectory
+ private Path workDir;
Review Comment:
We have a base Ignite test (`AbstractIgniteTest`?) that has a workDir in it
##########
modules/table/src/testFixtures/java/org/apache/ignite/distributed/ItTxTestCluster.java:
##########
@@ -218,7 +218,7 @@ public class ItTxTestCluster {
private ReplicaService clientReplicaSvc;
- protected Map<String, Loza> raftServers;
+ protected @Nullable Map<String, Loza> raftServers;
Review Comment:
Why isn't it initialized to an empty map?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -17,13 +17,29 @@
package org.apache.ignite.internal.network;
+import static java.util.stream.Collectors.toList;
+
+import java.net.InetAddress;
+import java.net.UnknownHostException;
+import java.util.Arrays;
import java.util.List;
+import org.apache.ignite.internal.logger.IgniteLogger;
+import org.apache.ignite.internal.logger.Loggers;
import org.apache.ignite.network.NetworkAddress;
/**
- * {@code NodeFinder} implementation that encapsulates a predefined list of
network addresses.
+ * {@code NodeFinder} implementation that encapsulates a predefined list of
network addresses and/or host names.
+ *
+ * <p>IP addresses are returned as is (up to a format change).
+ *
+ * <p>Names are resolved. If a name is resolved to at least one addreess, all
these addresses will be returned. If it resolves
Review Comment:
```suggestion
* <p>Names are resolved. If a name is resolved to one ore more addresses,
all of them will be returned. If it resolves
```
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -36,8 +52,37 @@ public StaticNodeFinder(List<NetworkAddress> addresses) {
this.addresses = addresses;
}
- /** {@inheritDoc} */
- @Override public List<NetworkAddress> findNodes() {
+ @Override
+ public List<NetworkAddress> findNodes() {
+ return addresses.parallelStream()
+ .flatMap(
+ originalAddress ->
Arrays.stream(resolveAll(originalAddress.host()))
+ .map(ip -> new NetworkAddress(ip,
originalAddress.port()))
+ )
+ .collect(toList());
+ }
+
+ private static String[] resolveAll(String host) {
+ InetAddress[] inetAddresses;
+ try {
+ inetAddresses = InetAddress.getAllByName(host);
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot resolve {}", host);
+ return new String[0];
Review Comment:
We have a constant for empty arrays)
##########
modules/network/src/test/java/org/apache/ignite/internal/network/StaticNodeFinderTest.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.network;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.InetAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.network.NetworkAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(WorkDirectoryExtension.class)
+class StaticNodeFinderTest {
+ @WorkDirectory
+ private Path workDir;
+
+ @Test
+ void returnsIpAddresses() {
+ NetworkAddress ipv4 = new NetworkAddress("1.2.3.4", 3001);
+ NetworkAddress ipv6 = new
NetworkAddress("2001:1234:130f:1234:1234:99c0:876a:130b", 3002);
+ NodeFinder finder = new StaticNodeFinder(List.of(ipv4, ipv6));
+
+ assertThat(finder.findNodes(), contains(ipv4, ipv6));
+ }
+
+ @Test
+ void resolvesLocalHostToJustOneAddress() throws Exception {
+ NodeFinder finder = new StaticNodeFinder(List.of(new
NetworkAddress("localhost", 3001)));
Review Comment:
How do you guarantee that there's more than one address to choose from?
##########
modules/table/src/testFixtures/java/org/apache/ignite/internal/table/TxAbstractTest.java:
##########
@@ -330,7 +330,12 @@ protected TxManager txManager(TableViewInternal t) {
protected boolean assertPartitionsSame(TableViewInternal table, int
partId) {
long storageIdx = 0;
- for (Map.Entry<String, Loza> entry :
txTestCluster.raftServers().entrySet()) {
+ Map<String, Loza> raftServers = txTestCluster.raftServers();
+ if (raftServers == null) {
Review Comment:
I wonder how you stumbled upon this)
##########
modules/network/src/test/java/org/apache/ignite/internal/network/StaticNodeFinderTest.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.network;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.InetAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.network.NetworkAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(WorkDirectoryExtension.class)
+class StaticNodeFinderTest {
+ @WorkDirectory
+ private Path workDir;
+
+ @Test
+ void returnsIpAddresses() {
+ NetworkAddress ipv4 = new NetworkAddress("1.2.3.4", 3001);
+ NetworkAddress ipv6 = new
NetworkAddress("2001:1234:130f:1234:1234:99c0:876a:130b", 3002);
+ NodeFinder finder = new StaticNodeFinder(List.of(ipv4, ipv6));
+
+ assertThat(finder.findNodes(), contains(ipv4, ipv6));
+ }
+
+ @Test
+ void resolvesLocalHostToJustOneAddress() throws Exception {
+ NodeFinder finder = new StaticNodeFinder(List.of(new
NetworkAddress("localhost", 3001)));
+
+ List<NetworkAddress> nodes = finder.findNodes();
+
+ assertThat(nodes, hasSize(1));
+ InetAddress inetAddress = InetAddress.getByName(nodes.get(0).host());
+ assertTrue(inetAddress.isLoopbackAddress(), nodes.get(0).host() + "
must be local");
+ }
+
+ @Test
+ void resolvesNames() throws Exception {
+ Path hostsFilePath = workDir.resolve("hosts");
+ Files.writeString(
+ hostsFilePath,
+ hostsFileContent(Map.of(
+ "1.2.3.4", "abc.def",
+ "1.2.3.5", "abc.def",
+ "4.3.2.1", "def.abc"
+ ))
+ );
+
+ List<NetworkAddress> foundAddresses =
findAddressesWithOverriddenNameResolving(
+ List.of(new NetworkAddress("abc.def", 3001), new
NetworkAddress("def.abc", 3002)),
+ hostsFilePath
+ );
+
+ assertThat(
+ foundAddresses,
+ containsInAnyOrder(
+ new NetworkAddress("1.2.3.4", 3001),
+ new NetworkAddress("1.2.3.5", 3001),
+ new NetworkAddress("4.3.2.1", 3002)
+ )
+ );
+ }
+
+ @Test
+ void ignoresUnresolvableName() throws Exception {
+ Path hostsFilePath = workDir.resolve("hosts");
+ Files.writeString(hostsFilePath, hostsFileContent(Map.of("1.2.3.4",
"abc.def")));
+
+ List<NetworkAddress> foundAddresses =
findAddressesWithOverriddenNameResolving(
+ List.of(new NetworkAddress("abc.def", 3001), new
NetworkAddress("def.abc", 3002)),
+ hostsFilePath
+ );
+
+ assertThat(foundAddresses, contains(new NetworkAddress("1.2.3.4",
3001)));
+ }
+
+ /**
+ * Invokes {@link StaticNodeFinder} with name resolving overridden with
our name->ips mapping.
+ * Does this by starting {@link StaticNodeFinderMain} in another JVM to
allow jdk.net.hosts.file property
+ * override name resolving (this defines path to a file in /etc/hosts
format).
+ *
+ * @param addresses Addresses to initialize a {@link StaticNodeFinder}
with.
+ * @param hostsFilePath Path to hosts file (this file defines the
overrides).
+ * @return Found addresses.
+ * @throws IOException If something goes wrong.
+ * @throws InterruptedException If interrupted while waiting for another
JVM to terminate.
+ */
+ private static List<NetworkAddress>
findAddressesWithOverriddenNameResolving(
+ List<NetworkAddress> addresses,
+ Path hostsFilePath
+ ) throws IOException, InterruptedException {
+ String javaBinaryPath =
ProcessHandle.current().info().command().orElseThrow();
+ String javaClassPath = System.getProperty("java.class.path");
+
+ //noinspection UseOfProcessBuilder
+ ProcessBuilder processBuilder = new ProcessBuilder(
+ javaBinaryPath,
+ "-cp", javaClassPath,
+ "-Djdk.net.hosts.file=" + hostsFilePath,
Review Comment:
`System.setProperty` doesn't work for this?
##########
modules/network/src/test/java/org/apache/ignite/internal/network/StaticNodeFinderTest.java:
##########
@@ -0,0 +1,172 @@
+/*
+ * 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.network;
+
+import static java.nio.charset.StandardCharsets.UTF_8;
+import static java.util.concurrent.TimeUnit.SECONDS;
+import static java.util.stream.Collectors.joining;
+import static java.util.stream.Collectors.toList;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.contains;
+import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.hamcrest.Matchers.hasSize;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.InetAddress;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.Arrays;
+import java.util.List;
+import java.util.Map;
+import org.apache.ignite.internal.testframework.WorkDirectory;
+import org.apache.ignite.internal.testframework.WorkDirectoryExtension;
+import org.apache.ignite.network.NetworkAddress;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+
+@ExtendWith(WorkDirectoryExtension.class)
+class StaticNodeFinderTest {
+ @WorkDirectory
+ private Path workDir;
+
+ @Test
+ void returnsIpAddresses() {
+ NetworkAddress ipv4 = new NetworkAddress("1.2.3.4", 3001);
+ NetworkAddress ipv6 = new
NetworkAddress("2001:1234:130f:1234:1234:99c0:876a:130b", 3002);
+ NodeFinder finder = new StaticNodeFinder(List.of(ipv4, ipv6));
+
+ assertThat(finder.findNodes(), contains(ipv4, ipv6));
+ }
+
+ @Test
+ void resolvesLocalHostToJustOneAddress() throws Exception {
+ NodeFinder finder = new StaticNodeFinder(List.of(new
NetworkAddress("localhost", 3001)));
+
+ List<NetworkAddress> nodes = finder.findNodes();
+
+ assertThat(nodes, hasSize(1));
+ InetAddress inetAddress = InetAddress.getByName(nodes.get(0).host());
+ assertTrue(inetAddress.isLoopbackAddress(), nodes.get(0).host() + "
must be local");
+ }
+
+ @Test
+ void resolvesNames() throws Exception {
+ Path hostsFilePath = workDir.resolve("hosts");
+ Files.writeString(
+ hostsFilePath,
+ hostsFileContent(Map.of(
+ "1.2.3.4", "abc.def",
+ "1.2.3.5", "abc.def",
+ "4.3.2.1", "def.abc"
+ ))
+ );
+
+ List<NetworkAddress> foundAddresses =
findAddressesWithOverriddenNameResolving(
+ List.of(new NetworkAddress("abc.def", 3001), new
NetworkAddress("def.abc", 3002)),
+ hostsFilePath
+ );
+
+ assertThat(
+ foundAddresses,
+ containsInAnyOrder(
+ new NetworkAddress("1.2.3.4", 3001),
+ new NetworkAddress("1.2.3.5", 3001),
+ new NetworkAddress("4.3.2.1", 3002)
+ )
+ );
+ }
+
+ @Test
+ void ignoresUnresolvableName() throws Exception {
+ Path hostsFilePath = workDir.resolve("hosts");
+ Files.writeString(hostsFilePath, hostsFileContent(Map.of("1.2.3.4",
"abc.def")));
+
+ List<NetworkAddress> foundAddresses =
findAddressesWithOverriddenNameResolving(
+ List.of(new NetworkAddress("abc.def", 3001), new
NetworkAddress("def.abc", 3002)),
+ hostsFilePath
+ );
+
+ assertThat(foundAddresses, contains(new NetworkAddress("1.2.3.4",
3001)));
+ }
+
+ /**
+ * Invokes {@link StaticNodeFinder} with name resolving overridden with
our name->ips mapping.
+ * Does this by starting {@link StaticNodeFinderMain} in another JVM to
allow jdk.net.hosts.file property
+ * override name resolving (this defines path to a file in /etc/hosts
format).
+ *
+ * @param addresses Addresses to initialize a {@link StaticNodeFinder}
with.
+ * @param hostsFilePath Path to hosts file (this file defines the
overrides).
+ * @return Found addresses.
+ * @throws IOException If something goes wrong.
+ * @throws InterruptedException If interrupted while waiting for another
JVM to terminate.
+ */
+ private static List<NetworkAddress>
findAddressesWithOverriddenNameResolving(
+ List<NetworkAddress> addresses,
+ Path hostsFilePath
+ ) throws IOException, InterruptedException {
+ String javaBinaryPath =
ProcessHandle.current().info().command().orElseThrow();
+ String javaClassPath = System.getProperty("java.class.path");
+
+ //noinspection UseOfProcessBuilder
+ ProcessBuilder processBuilder = new ProcessBuilder(
+ javaBinaryPath,
+ "-cp", javaClassPath,
+ "-Djdk.net.hosts.file=" + hostsFilePath,
+ StaticNodeFinderMain.class.getName(),
+
addresses.stream().map(NetworkAddress::toString).collect(joining(","))
+ );
+ Process process = processBuilder.start();
+
+ if (!process.waitFor(10, SECONDS)) {
+ throw new RuntimeException("Process did not finish in 10 seconds");
+ }
+ if (process.exitValue() != 0) {
+ throw new RuntimeException("Return code " + process.exitValue()
+ + ", stdout: " + stdoutString(process) + ", stderr: " +
stderrString(process));
+ }
+
+ String output = stdoutString(process);
+ return Arrays.stream(output.split(","))
+ .map(NetworkAddress::from)
+ .collect(toList());
+ }
+
+ private static String hostsFileContent(Map<String, String> ipToHostname) {
+ return ipToHostname.entrySet().stream()
+ .map(entry -> entry.getKey() + " " + entry.getValue())
+ .collect(joining("\n"));
+ }
+
+ private static String stdoutString(Process process) {
+ try (InputStream stdout = process.getInputStream()) {
+ return new String(stdout.readAllBytes(), UTF_8);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review Comment:
I think you simply propagate these exceptions
--
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]
