You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "sashapolo (via GitHub)" <gi...@apache.org> on 2026/01/27 11:48:42 UTC
[PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
sashapolo opened a new pull request, #7483:
URL: https://github.com/apache/ignite-3/pull/7483
https://issues.apache.org/jira/browse/IGNITE-27672
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2734909450
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -84,47 +112,53 @@ public void start() {
// No-op
}
- private static String[] resolveAll(String host) {
- InetAddress[] inetAddresses = null;
+ private List<NetworkAddress> resolveAddress(NetworkAddress address) {
+ InetAddress[] inetAddresses = resolveInetAddresses(address.host());
- int tryCount = 0;
- boolean resolved = false;
+ if (inetAddresses.length == 0) {
+ return List.of();
+ }
- do {
- tryCount++;
+ var addresses = new ArrayList<NetworkAddress>(inetAddresses.length);
- try {
- inetAddresses = InetAddress.getAllByName(host);
- resolved = true;
- } catch (UnknownHostException e) {
- if (tryCount == MAX_TRIES) {
- LOG.warn("Cannot resolve {}", host);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
+ for (InetAddress inetAddress : inetAddresses) {
+ if (inetAddress.isLoopbackAddress()) {
+ // If it's a loopback address (like 127.0.0.1), only return this address.
+ return List.of(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ addresses.add(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ return addresses;
+ }
+
+ private InetAddress[] resolveInetAddresses(String host) {
+ try {
+ int tryCount = 1;
+ while (true) {
try {
- Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
+ return hostNameResolver.getAllByName(host);
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot resolve host \"{}\" (attempt {}/{}).", host, tryCount, nameResolutionAttempts);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
- }
- } while (!resolved);
+ if (tryCount >= nameResolutionAttempts) {
+ break;
+ }
- assert inetAddresses != null;
- String[] addresses = new String[inetAddresses.length];
- for (int i = 0; i < inetAddresses.length; i++) {
- InetAddress inetAddress = inetAddresses[i];
+ //noinspection BusyWait
+ Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
Review Comment:
So for default 10 tries we will wait for total of 27500 millis, is it desirable?
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -84,47 +112,53 @@ public void start() {
// No-op
}
- private static String[] resolveAll(String host) {
- InetAddress[] inetAddresses = null;
+ private List<NetworkAddress> resolveAddress(NetworkAddress address) {
+ InetAddress[] inetAddresses = resolveInetAddresses(address.host());
- int tryCount = 0;
- boolean resolved = false;
+ if (inetAddresses.length == 0) {
+ return List.of();
+ }
- do {
- tryCount++;
+ var addresses = new ArrayList<NetworkAddress>(inetAddresses.length);
- try {
- inetAddresses = InetAddress.getAllByName(host);
- resolved = true;
- } catch (UnknownHostException e) {
- if (tryCount == MAX_TRIES) {
- LOG.warn("Cannot resolve {}", host);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
+ for (InetAddress inetAddress : inetAddresses) {
+ if (inetAddress.isLoopbackAddress()) {
+ // If it's a loopback address (like 127.0.0.1), only return this address.
+ return List.of(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ addresses.add(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ return addresses;
+ }
+
+ private InetAddress[] resolveInetAddresses(String host) {
+ try {
+ int tryCount = 1;
+ while (true) {
try {
- Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
+ return hostNameResolver.getAllByName(host);
Review Comment:
InetAddess.getAllByName by default uses cache, from the code it looks like until value is expired we will get the same value with the same UnknownHostException
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -44,19 +44,50 @@
*/
public class StaticNodeFinder implements NodeFinder {
private static final IgniteLogger LOG = Loggers.forClass(StaticNodeFinder.class);
+
private static final long RETRY_WAIT_BASE_MILLIS = 500;
- private static final int MAX_TRIES = 3;
/** List of seed cluster members. */
- private final List<NetworkAddress> addresses;
+ private final Set<NetworkAddress> addresses;
+
+ private final int nameResolutionAttempts;
+
+ private final HostNameResolver hostNameResolver;
+
+ /**
+ * Class for resolving host names.
+ *
+ * <p>Needed for writing cleaner tests.
+ */
+ @FunctionalInterface
+ public interface HostNameResolver {
+ /**
+ * Given the name of a host, returns an array of its IP addresses, based on the configured name service on the system.
+ */
+ InetAddress[] getAllByName(String host) throws UnknownHostException;
+ }
/**
* Constructor.
*
* @param addresses Addresses of initial cluster members.
*/
+ @TestOnly
Review Comment:
If we pass hostNameResolver just for tests, should we make constructor with this param a TestOnly, and constuctor using InetAddress:getAllByName a production one?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo merged PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2735884186
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -84,47 +112,53 @@ public void start() {
// No-op
}
- private static String[] resolveAll(String host) {
- InetAddress[] inetAddresses = null;
+ private List<NetworkAddress> resolveAddress(NetworkAddress address) {
+ InetAddress[] inetAddresses = resolveInetAddresses(address.host());
- int tryCount = 0;
- boolean resolved = false;
+ if (inetAddresses.length == 0) {
+ return List.of();
+ }
- do {
- tryCount++;
+ var addresses = new ArrayList<NetworkAddress>(inetAddresses.length);
- try {
- inetAddresses = InetAddress.getAllByName(host);
- resolved = true;
- } catch (UnknownHostException e) {
- if (tryCount == MAX_TRIES) {
- LOG.warn("Cannot resolve {}", host);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
+ for (InetAddress inetAddress : inetAddresses) {
+ if (inetAddress.isLoopbackAddress()) {
+ // If it's a loopback address (like 127.0.0.1), only return this address.
+ return List.of(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ addresses.add(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ return addresses;
+ }
+
+ private InetAddress[] resolveInetAddresses(String host) {
+ try {
+ int tryCount = 1;
+ while (true) {
try {
- Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
+ return hostNameResolver.getAllByName(host);
+ } catch (UnknownHostException e) {
+ LOG.warn("Cannot resolve host \"{}\" (attempt {}/{}).", host, tryCount, nameResolutionAttempts);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
- }
- } while (!resolved);
+ if (tryCount >= nameResolutionAttempts) {
+ break;
+ }
- assert inetAddresses != null;
- String[] addresses = new String[inetAddresses.length];
- for (int i = 0; i < inetAddresses.length; i++) {
- InetAddress inetAddress = inetAddresses[i];
+ //noinspection BusyWait
+ Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
Review Comment:
it's not 27500 ms, it's 22500, which I think is reasonable, no?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "Phillippko (via GitHub)" <gi...@apache.org>.
Phillippko commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2735970171
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -84,47 +112,53 @@ public void start() {
// No-op
}
- private static String[] resolveAll(String host) {
- InetAddress[] inetAddresses = null;
+ private List<NetworkAddress> resolveAddress(NetworkAddress address) {
+ InetAddress[] inetAddresses = resolveInetAddresses(address.host());
- int tryCount = 0;
- boolean resolved = false;
+ if (inetAddresses.length == 0) {
+ return List.of();
+ }
- do {
- tryCount++;
+ var addresses = new ArrayList<NetworkAddress>(inetAddresses.length);
- try {
- inetAddresses = InetAddress.getAllByName(host);
- resolved = true;
- } catch (UnknownHostException e) {
- if (tryCount == MAX_TRIES) {
- LOG.warn("Cannot resolve {}", host);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
+ for (InetAddress inetAddress : inetAddresses) {
+ if (inetAddress.isLoopbackAddress()) {
+ // If it's a loopback address (like 127.0.0.1), only return this address.
+ return List.of(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ addresses.add(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ return addresses;
+ }
+
+ private InetAddress[] resolveInetAddresses(String host) {
+ try {
+ int tryCount = 1;
+ while (true) {
try {
- Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
+ return hostNameResolver.getAllByName(host);
Review Comment:
I see, so it adds the address to cache, but then removes if got an 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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2735832295
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -44,19 +44,50 @@
*/
public class StaticNodeFinder implements NodeFinder {
private static final IgniteLogger LOG = Loggers.forClass(StaticNodeFinder.class);
+
private static final long RETRY_WAIT_BASE_MILLIS = 500;
- private static final int MAX_TRIES = 3;
/** List of seed cluster members. */
- private final List<NetworkAddress> addresses;
+ private final Set<NetworkAddress> addresses;
+
+ private final int nameResolutionAttempts;
+
+ private final HostNameResolver hostNameResolver;
+
+ /**
+ * Class for resolving host names.
+ *
+ * <p>Needed for writing cleaner tests.
+ */
+ @FunctionalInterface
+ public interface HostNameResolver {
+ /**
+ * Given the name of a host, returns an array of its IP addresses, based on the configured name service on the system.
+ */
+ InetAddress[] getAllByName(String host) throws UnknownHostException;
+ }
/**
* Constructor.
*
* @param addresses Addresses of initial cluster members.
*/
+ @TestOnly
Review Comment:
I thought about that but was too lazy
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2731670073
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -44,19 +44,49 @@
*/
public class StaticNodeFinder implements NodeFinder {
private static final IgniteLogger LOG = Loggers.forClass(StaticNodeFinder.class);
+
private static final long RETRY_WAIT_BASE_MILLIS = 500;
- private static final int MAX_TRIES = 3;
/** List of seed cluster members. */
- private final List<NetworkAddress> addresses;
+ private final Set<NetworkAddress> addresses;
+
+ private final int nameResolutionAttempts;
+
+ private final HostNameResolver hostNameResolver;
+
+ /**
+ * Class for resolving host names.
+ *
+ * <p>Needed for writing cleaner tests.
+ */
+ @FunctionalInterface
+ public interface HostNameResolver {
+ /**
+ * Given the name of a host, returns an array of its IP addresses, based on the configured name service on the system.
+ */
+ InetAddress[] getAllByName(String host) throws UnknownHostException;
+ }
/**
* Constructor.
*
* @param addresses Addresses of initial cluster members.
*/
+ @TestOnly
public StaticNodeFinder(List<NetworkAddress> addresses) {
- this.addresses = addresses;
+ this(addresses, InetAddress::getAllByName, 1);
+ }
+
+ /**
+ * Constructor.
+ *
+ * @param addresses Addresses of initial cluster members.
Review Comment:
The Javadoc for this constructor is now out of sync with the method signature: there is no `@param` documentation for `hostNameResolver`, and the existing `@param nameResolutionAttempts` tag does not match the parameter order. Please update the Javadoc to document `hostNameResolver` and reorder the `@param` tags to correspond to the actual parameter list.
```suggestion
* @param addresses Addresses of initial cluster members.
* @param hostNameResolver Resolver used to resolve host names from the {@code addresses} list.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-27672 Make StaticNodeFinder name resolution configurable [ignite-3]
Posted by "sashapolo (via GitHub)" <gi...@apache.org>.
sashapolo commented on code in PR #7483:
URL: https://github.com/apache/ignite-3/pull/7483#discussion_r2735932408
##########
modules/network/src/main/java/org/apache/ignite/internal/network/StaticNodeFinder.java:
##########
@@ -84,47 +112,53 @@ public void start() {
// No-op
}
- private static String[] resolveAll(String host) {
- InetAddress[] inetAddresses = null;
+ private List<NetworkAddress> resolveAddress(NetworkAddress address) {
+ InetAddress[] inetAddresses = resolveInetAddresses(address.host());
- int tryCount = 0;
- boolean resolved = false;
+ if (inetAddresses.length == 0) {
+ return List.of();
+ }
- do {
- tryCount++;
+ var addresses = new ArrayList<NetworkAddress>(inetAddresses.length);
- try {
- inetAddresses = InetAddress.getAllByName(host);
- resolved = true;
- } catch (UnknownHostException e) {
- if (tryCount == MAX_TRIES) {
- LOG.warn("Cannot resolve {}", host);
- return ArrayUtils.STRING_EMPTY_ARRAY;
- }
+ for (InetAddress inetAddress : inetAddresses) {
+ if (inetAddress.isLoopbackAddress()) {
+ // If it's a loopback address (like 127.0.0.1), only return this address.
+ return List.of(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ addresses.add(new NetworkAddress(inetAddress.getHostAddress(), address.port()));
+ }
+
+ return addresses;
+ }
+
+ private InetAddress[] resolveInetAddresses(String host) {
+ try {
+ int tryCount = 1;
+ while (true) {
try {
- Thread.sleep(tryCount * RETRY_WAIT_BASE_MILLIS);
- } catch (InterruptedException ex) {
- Thread.currentThread().interrupt();
+ return hostNameResolver.getAllByName(host);
Review Comment:
> from the code it looks like until value is expired we will get the same value with the same UnknownHostException
Why do you think so? I've had a look at the code and I can see this:
```
try {
inetAddresses = getAddressesFromNameService(host, reqAddr);
ex = null;
cachePolicy = InetAddressCachePolicy.get();
} catch (UnknownHostException uhe) {
inetAddresses = null;
ex = uhe;
cachePolicy = InetAddressCachePolicy.getNegative();
}
// remove or replace us with cached addresses according to cachePolicy
if (cachePolicy == InetAddressCachePolicy.NEVER) {
cache.remove(host, this);
```
So, in case of `UnknownHostException` we will use the `InetAddressCachePolicy.NEVER` which means that this result will not be cached.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org