chihsuan commented on code in PR #10831:
URL: https://github.com/apache/ozone/pull/10831#discussion_r3630122547
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/SafeModeCheckSubcommand.java:
##########
@@ -104,16 +104,16 @@ private SCMNodeInfo findLeaderNode(ScmClient scmClient)
throws IOException {
try {
List<String> roles = scmClient.getScmRoles();
for (String role : roles) {
- String[] parts = role.split(":");
- if (parts.length < 3 || !"LEADER".equalsIgnoreCase(parts[2])) {
+ String[] parts = HddsUtils.parseRatisRoleString(role);
+ if (!"LEADER".equalsIgnoreCase(parts[2])) {
continue;
}
String leaderHost = parts[0];
- String leaderIp = parts.length >= 5 ? parts[4] : null;
+ String leaderIp = parts[4];
for (SCMNodeInfo node : nodes) {
- String nodeHost = node.getScmClientAddress().split(":")[0];
+ String nodeHost =
HddsUtils.getHostName(node.getScmClientAddress()).orElse("");
Review Comment:
This correctly extracts the IPv6 host, but `matchesAddress()` still parses
its inputs with split`(":", 2)`. Equivalent forms such as `2001:db8::1` and
`2001:db8:0:0:0:0:0:1` therefore do not match. Could we make `matchesAddress()`
IPv6-aware as part of this fix?
https://github.com/AdyChechani/ozone/blob/0530be6310670bea08e54d711d929b7214cc25b0/hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/hdds/scm/cli/SafeModeCheckSubcommand.java#L189-L190
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/scm/GetScmRatisRolesSubcommand.java:
##########
@@ -92,20 +87,12 @@ private Map<String, Map<String, String>> parseScmRoles(
Map<String, Map<String, String>> allRoles = new HashMap<>();
for (String role : peerRoles) {
Map<String, String> roleDetails = new HashMap<>();
- String[] roles = role.split(":");
- if (roles.length < 2) {
- err.println("Invalid response received for ScmRatisRoles.");
- return Collections.emptyMap();
- }
- // In case, there is no ratis, there is no ratis role.
- // This will just print the hostname with ratis port as the address
- roleDetails.put("address", roles[0].concat(":").concat(roles[1]));
- if (roles.length == 5) {
- roleDetails.put("raftPeerRole", roles[2]);
- roleDetails.put("ID", roles[3]);
- roleDetails.put("InetAddress", roles[4]);
- }
- allRoles.put(roles[0], roleDetails);
+ String[] fields = HddsUtils.parseRatisRoleString(role);
Review Comment:
Should we preserve the existing `host:port` handling here? The previous code
explicitly supported this non-Ratis/two-field response, but the new parser
requires all five fields and crashes on `"host:9894"`. This looks like an
unintended behavior change.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java:
##########
@@ -289,13 +289,24 @@ public List<String> getRatisRoles() {
LOG.error("SCM Ratis PeerInetAddress {} is unresolvable",
peer.getAddress());
}
- ratisRoles.add((peer.getAddress() == null ? "" :
- peer.getAddress().concat(peer.equals(leader) ?
- ":".concat(RaftProtos.RaftPeerRole.LEADER.toString()) :
- ":".concat(RaftProtos.RaftPeerRole.FOLLOWER.toString()))
- .concat(":".concat(peer.getId().toString()))
- .concat(":".concat(peerInetAddress == null ? "" :
- peerInetAddress.getHostAddress()))));
+ if (peer.getAddress() == null) {
+ ratisRoles.add("");
+ continue;
+ }
+ String host = HddsUtils.getHostName(peer.getAddress()).orElse("");
+ int port = HddsUtils.getHostPort(peer.getAddress()).orElse(0);
+ String normalizedAddress = HddsUtils.getHostPortString(host, port);
+ String role = peer.equals(leader)
+ ? RaftProtos.RaftPeerRole.LEADER.toString()
+ : RaftProtos.RaftPeerRole.FOLLOWER.toString();
+ String hostIp = "";
+ if (peerInetAddress != null) {
+ String rawIp = peerInetAddress.getHostAddress();
+ hostIp = rawIp.contains(":") ? "[" + rawIp + "]" : rawIp;
Review Comment:
Small suggestion: since we already have the `InetAddress`,
`InetAddresses.toUriString(peerInetAddress)` handles this directly—IPv4 stays
unchanged and IPv6 is bracketed. That may make the formatting intent a little
clearer.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/HddsUtils.java:
##########
@@ -243,6 +243,52 @@ public static String getHostPortString(String host, int
port) {
return HostAndPort.fromParts(host, port).toString();
}
+ /**
+ * Parse a Ratis role string produced by
+ * {@code SCMRatisServerImpl.getRatisRoles()} into its constituent fields.
+ * The format is {@code [host]:port:ROLE:id:hostIP} where host and hostIP
+ * may be bracketed IPv6 literals.
+ *
+ * @param roleString the encoded role string
+ * @return a 5-element array: {host, port, role, id, hostIP}
+ */
+ public static String[] parseRatisRoleString(String roleString) {
+ // Parse from the right: the last field is hostIP (possibly bracketed),
+ // then id (uuid, no colons), then role (LEADER/FOLLOWER, no colons),
+ // and the remainder is host:port (which may be bracketed IPv6).
+ int idx = roleString.length();
+
+ // Field 5: hostIP — may be bracketed IPv6 like [2001:db8::1]
+ String hostIp;
+ if (idx > 0 && roleString.charAt(idx - 1) == ']') {
+ int bracket = roleString.lastIndexOf('[');
+ hostIp = roleString.substring(bracket + 1, idx - 1);
+ idx = bracket - 1; // skip the ':' before '['
+ } else {
+ int sep = roleString.lastIndexOf(':');
+ hostIp = roleString.substring(sep + 1);
+ idx = sep;
+ }
+
+ // Field 4: id (uuid or peer id, no colons)
+ int sep3 = roleString.lastIndexOf(':', idx - 1);
+ String id = roleString.substring(sep3 + 1, idx);
Review Comment:
Could we validate the input before calling `substring()`? Inputs such as
`""` or `"host:9894"` currently produce `StringIndexOutOfBoundsException` but
older SCMs can actually emit.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/ha/SCMRatisServerImpl.java:
##########
@@ -289,13 +289,24 @@ public List<String> getRatisRoles() {
LOG.error("SCM Ratis PeerInetAddress {} is unresolvable",
peer.getAddress());
}
- ratisRoles.add((peer.getAddress() == null ? "" :
- peer.getAddress().concat(peer.equals(leader) ?
- ":".concat(RaftProtos.RaftPeerRole.LEADER.toString()) :
- ":".concat(RaftProtos.RaftPeerRole.FOLLOWER.toString()))
- .concat(":".concat(peer.getId().toString()))
- .concat(":".concat(peerInetAddress == null ? "" :
- peerInetAddress.getHostAddress()))));
+ if (peer.getAddress() == null) {
+ ratisRoles.add("");
+ continue;
+ }
+ String host = HddsUtils.getHostName(peer.getAddress()).orElse("");
+ int port = HddsUtils.getHostPort(peer.getAddress()).orElse(0);
Review Comment:
Is a missing Ratis port valid here? Defaulting to 0 makes the response look
valid and consumers may display it as the actual port. If the port is required,
failing with a clear message may be safer than silently using 0.
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/scm/GetScmRatisRolesSubcommand.java:
##########
@@ -92,20 +87,12 @@ private Map<String, Map<String, String>> parseScmRoles(
Map<String, Map<String, String>> allRoles = new HashMap<>();
for (String role : peerRoles) {
Map<String, String> roleDetails = new HashMap<>();
- String[] roles = role.split(":");
- if (roles.length < 2) {
- err.println("Invalid response received for ScmRatisRoles.");
- return Collections.emptyMap();
- }
- // In case, there is no ratis, there is no ratis role.
- // This will just print the hostname with ratis port as the address
- roleDetails.put("address", roles[0].concat(":").concat(roles[1]));
- if (roles.length == 5) {
- roleDetails.put("raftPeerRole", roles[2]);
- roleDetails.put("ID", roles[3]);
- roleDetails.put("InetAddress", roles[4]);
- }
- allRoles.put(roles[0], roleDetails);
+ String[] fields = HddsUtils.parseRatisRoleString(role);
+ roleDetails.put("address", fields[0] + ":" + fields[1]);
Review Comment:
For IPv6 this produces an ambiguous address such as `2001:db8::1:9894`.
Could we use `HddsUtils.getHostPortString(...)` here so the JSON output remains
`[2001:db8::1]:9894`?
##########
hadoop-ozone/cli-admin/src/main/java/org/apache/hadoop/ozone/admin/scm/GetScmRatisRolesSubcommand.java:
##########
@@ -73,10 +71,7 @@ public void execute(ScmClient scmClient) throws IOException {
formattingCLIUtils.addHeaders(SCM_ROLES_HEADER);
for (String role : peerRoles) {
- String[] roleItems = role.split(":");
- if (roleItems.length < 2) {
- err.println("Invalid response received for ScmRatisRoles.");
- }
+ String[] roleItems = HddsUtils.parseRatisRoleString(role);
Review Comment:
The table path previously handled short or invalid responses without
throwing. Since the new parser requires five fields, should we preserve the
existing validation/skip behavior here?
--
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]
