HDDS-15682. Use the original configured host and port to identify SCM nodes by szetszwo · Pull Request #10612 · apache/ozone

szetszwo · 2026-06-25T21:44:58Z

What changes were proposed in this pull request?

The data structures such as SCMConnectionManager.scmMachines, StateContext.endpoint, etc. use InetSocketAddress to identify SCM nodes. Since the InetSocketAddress can changed in some environment such as Kubernetes, it is better to use the original configured host and port instead of the resolved InetSocketAddress.

What is the link to the Apache JIRA

HDDS-15682

How was this patch tested?

Updating existing tests.

… nodes.

kerneltime

More incoming.

kerneltime · 2026-06-26T05:37:32Z


  public String getBlockClientAddress() {
-    return blockClientAddress;
+    return blockClientAddress.getHostAndPortString();


Suggested change

return blockClientAddress.getHostAndPortString();

return blockClientAddress == null ? null : blockClientAddress.getHostAndPortString();

These HostAndPort variables in SCMNodeInfo are never null. Let's add Objects.requireNonNull in the constructor instead.

kerneltime · 2026-06-26T05:37:52Z


  public String getScmClientAddress() {
-    return scmClientAddress;
+    return scmClientAddress.getHostAndPortString();


Suggested change

return scmClientAddress.getHostAndPortString();

return scmClientAddress == null ? null : scmClientAddress.getHostAndPortString();

For comments along the same line, it is better to suggest one change and mention that the suggestion applies to other parts of the code. This makes the review less verbose, plus others do not need to respond to each similar comment.

kerneltime · 2026-06-26T05:38:13Z


  public String getScmSecurityAddress() {
-    return scmSecurityAddress;
+    return scmSecurityAddress.getHostAndPortString();


Suggested change

return scmSecurityAddress.getHostAndPortString();

return scmSecurityAddress == null ? null : scmSecurityAddress.getHostAndPortString();

kerneltime · 2026-06-26T05:38:45Z

  }
+
+  public String getScmDatanodeAddress() {
+    return scmDatanodeAddress.getHostAndPortString();


Suggested change

return scmDatanodeAddress.getHostAndPortString();

return scmDatanodeAddress == null ? null : scmDatanodeAddress.getHostAndPortString();

kerneltime · 2026-06-26T05:39:56Z

+    return hostAndPortString;
+  }
+
+  public synchronized InetSocketAddress getAddress() {


why is this synchronized?

It is for HDDS-15533, which will update it. Actually, let's remove it here and add it there.

kerneltime

Keying SCM identity on configured host+port rather than the resolved InetSocketAddress reads as a sound fix for the IP-churn problem. A few non-blocking notes from an AI-assisted review pass below — all minor/cleanup, nothing that blocks merge.

kerneltime · 2026-06-26T05:54:56Z

+ * A class for host and port.
+ * It also has an address which can be updated from time to time.
+ */
+public class HostAndPort {


Minor (naming): this collides with Guava's com.google.common.net.HostAndPort, which is already imported in this same module at HddsUtils.java:36. It also lives in scm.net, which package-info scopes to network topology (NetworkTopology/Node/…) and which already has its own NetUtils — so the import org.apache.hadoop.net.NetUtils here shadows the same-package type. A more specific name/home (e.g. ScmEndpoint) would avoid both. Cheap while the type is new.

— Claude Code (AI-assisted review)

We can replace Guava's HostAndPort with this. (Can be done in follow-up if considered out-of-scope.)

kerneltime · 2026-06-26T05:54:56Z

+    this.port = port;
+    this.hostAndPortString = host + ":" + port;
+    this.hash = host.hashCode() ^ Integer.hashCode(port);
+    this.address = address != null ? address : NetUtils.createSocketAddr(hostAndPortString);


The constructor resolves eagerly. For the block/client/security addresses in SCMNodeInfo the resolved value is never read — those getters return only getHostAndPortString(), and the string consumers re-resolve it (e.g. HddsUtils.getScmAddressForClients and StorageContainerManager both call NetUtils.createSocketAddr on it again) — so each ends up resolved twice and this result is discarded. Resolving lazily inside getAddress() would drop the duplicate lookup and also fits the "address can be updated from time to time" intent.

— Claude Code (AI-assisted review)

Use InetSocketAddress.createUnresolved(host, port) directly instead of Hadoop's NetUtils.createSocketAddr. The latter parses the string, into host and port (so we can save this processing), and handles more complex cases that we do not need here.

The constructor resolves eagerly ...

Use InetSocketAddress.createUnresolved(host, port) directly ...

Let's change the address resolution logic in HDDS-15533. This PR just do refactoring.

kerneltime · 2026-06-26T05:54:56Z

+    this(host, port, null);
+  }
+
+  public HostAndPort(InetSocketAddress address) {


equals/hashCode key on host, but this ctor derives host from address.getHostName() while the other uses the raw configured string. Those can differ (IP literal vs configured name), so two instances for the same node could be unequal and miss in the endpoint maps. It looks like this ctor is test-only today, so it's latent — but since stable host-string identity is the point of the change, might be worth dropping it or documenting that callers must use one consistent spelling.

— Claude Code (AI-assisted review)

This is only used in ContainerTestUtils. Let me change the code there.

kerneltime · 2026-06-26T05:54:56Z

  @Override
  public String getAddressString() {
-    return getAddress().toString();
+    return getAddress().getHostAndPortString();


getAddressString() is part of EndpointStateMachineMBean, so this shifts the JMX value from host/ip:port to host:port (drops the resolved IP). Intentional? Flagging only in case any monitoring parses it.

— Claude Code (AI-assisted review)

kerneltime · 2026-06-26T06:45:54Z

I addressed the review comments and redid the DNS fix at szetszwo#11 please take a look.

adoroszlai

Thanks @szetszwo for the patch.

adoroszlai · 2026-06-26T14:56:40Z


  public String getBlockClientAddress() {
-    return blockClientAddress;
+    return blockClientAddress.getHostAndPortString();


These HostAndPort variables in SCMNodeInfo are never null. Let's add Objects.requireNonNull in the constructor instead.

adoroszlai · 2026-06-26T15:00:23Z


  public String getScmClientAddress() {
-    return scmClientAddress;
+    return scmClientAddress.getHostAndPortString();


For comments along the same line, it is better to suggest one change and mention that the suggestion applies to other parts of the code. This makes the review less verbose, plus others do not need to respond to each similar comment.

adoroszlai · 2026-06-26T15:09:38Z

+    this.port = port;
+    this.hostAndPortString = host + ":" + port;
+    this.hash = host.hashCode() ^ Integer.hashCode(port);
+    this.address = address != null ? address : NetUtils.createSocketAddr(hostAndPortString);


Use InetSocketAddress.createUnresolved(host, port) directly instead of Hadoop's NetUtils.createSocketAddr. The latter parses the string, into host and port (so we can save this processing), and handles more complex cases that we do not need here.

adoroszlai · 2026-06-26T15:14:34Z

+ * A class for host and port.
+ * It also has an address which can be updated from time to time.
+ */
+public class HostAndPort {


We can replace Guava's HostAndPort with this. (Can be done in follow-up if considered out-of-scope.)

HDDS-15682. Use the original configured host and port to identify SCM…

bceb1a0

… nodes.

szetszwo requested a review from kerneltime June 25, 2026 21:45

szetszwo mentioned this pull request Jun 25, 2026

HDDS-15533. DNS refresh on heartbeat failure for DN to SCM #10488

Open

kerneltime reviewed Jun 26, 2026

View reviewed changes

kerneltime mentioned this pull request Jun 26, 2026

HDDS-15533. DNS refresh on heartbeat failure for DN to SCM (on HDDS-15682) szetszwo/ozone#11

Open

adoroszlai reviewed Jun 26, 2026

View reviewed changes

szetszwo added 3 commits June 26, 2026 12:20

Address review comments.

48f9ba9

Remove a HostAndPort constructor

81f05ff

Fix findbugs

1c4e33d

	return blockClientAddress.getHostAndPortString();
	return blockClientAddress == null ? null : blockClientAddress.getHostAndPortString();

	return scmClientAddress.getHostAndPortString();
	return scmClientAddress == null ? null : scmClientAddress.getHostAndPortString();

	return scmSecurityAddress.getHostAndPortString();
	return scmSecurityAddress == null ? null : scmSecurityAddress.getHostAndPortString();

	return scmDatanodeAddress.getHostAndPortString();
	return scmDatanodeAddress == null ? null : scmDatanodeAddress.getHostAndPortString();

Uh oh!

Conversation

szetszwo commented Jun 25, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

kerneltime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerneltime left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

szetszwo Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kerneltime commented Jun 26, 2026

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

szetszwo Jun 26, 2026 •

edited

Loading