smengcl commented on code in PR #11619:
URL: https://github.com/apache/ozone/pull/11619#discussion_r4319246958
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerSubCommandUtil.java:
##########
@@ -168,14 +168,9 @@ public static DiskBalancerProtocol
getSingleNodeDiskBalancerProxy(
UserGroupInformation user = UserGroupInformation.getCurrentUser();
// Parse address and add default port if not specified
- InetSocketAddress nodeAddr;
- if (address.contains(":")) {
- // Port is specified, use NetUtils to parse
- nodeAddr = NetUtils.createSocketAddr(address);
- } else {
- // Port not specified, use default
- nodeAddr = NetUtils.createSocketAddr(address,
HDDS_DATANODE_CLIENT_PORT_DEFAULT);
- }
+ int port =
HddsUtils.getHostPort(address).orElse(HDDS_DATANODE_CLIENT_PORT_DEFAULT);
+ InetSocketAddress nodeAddr = NetUtils.createSocketAddr(
Review Comment:
Thanks, good catch. `getSingleNodeDiskBalancerProxy` now calls
`HddsUtils.validateHostPortAuthority` (made public) before parsing, so
`2001:db8::1:1234` and a bare `2001:db8::1` are rejected with a message
suggesting `[2001:db8::1]:1234` or `[2001:db8::1:1234]`. Added negative cases
in `TestDiskBalancerSubCommandUtil`.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/datanode/DiskBalancerSubCommandUtil.java:
##########
@@ -168,14 +168,9 @@ public static DiskBalancerProtocol
getSingleNodeDiskBalancerProxy(
UserGroupInformation user = UserGroupInformation.getCurrentUser();
// Parse address and add default port if not specified
- InetSocketAddress nodeAddr;
- if (address.contains(":")) {
- // Port is specified, use NetUtils to parse
- nodeAddr = NetUtils.createSocketAddr(address);
- } else {
- // Port not specified, use default
- nodeAddr = NetUtils.createSocketAddr(address,
HDDS_DATANODE_CLIENT_PORT_DEFAULT);
- }
+ int port =
HddsUtils.getHostPort(address).orElse(HDDS_DATANODE_CLIENT_PORT_DEFAULT);
+ InetSocketAddress nodeAddr = NetUtils.createSocketAddr(
Review Comment:
Done, using the same check: `HddsUtils.validateHostPortAuthority` is the
part of `validateAdvertisedAddress` that rejects unbracketed literals. I did
not use `validateAdvertisedAddress` itself because its wildcard and link local
checks are about advertised addresses, not a CLI target.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMHANodeDetails.java:
##########
@@ -232,7 +232,8 @@ public static SCMHANodeDetails
loadSCMHAConfig(OzoneConfiguration conf,
InetSocketAddress addr = null;
try {
- addr = NetUtils.createSocketAddr(rpcAddrStr, ratisPort);
+ addr =
NetUtils.createSocketAddr(HddsUtils.getHostPortString(HddsUtils.getHostName(rpcAddrStr).get(),
Review Comment:
I left this one unchanged. `ozone.scm.address.<serviceId>.<nodeId>` is a
host only property: `SCMNodeInfo` (via `validateAdvertisedHost`) appends each
SCM port to the whole value, so a bare `2001:db8::1:1234` is already read as
the host `2001:db8::1:1234` there, and this path now reads it the same way.
Rejecting unbracketed literals only here would make SCM HA refuse a value that
clients and datanodes accept. If we want to require brackets for this property,
I think it should be done in `validateAdvertisedHost` for all readers in a
separate change.
_🤖 Addressed by [Claude Code](https://claude.com/claude-code)_
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
