Copilot commented on code in PR #10933:
URL: https://github.com/apache/ozone/pull/10933#discussion_r3705012563
##########
hadoop-ozone/ozonefs-common/src/test/java/org/apache/hadoop/fs/ozone/TestBasicOzoneFileSystems.java:
##########
@@ -137,6 +143,37 @@ public void testCreateSnapshotReturnPath(
}
}
+ @ParameterizedTest
+ @CsvSource(value = {
+ // hostname / IPv4 authority (behaviour unchanged)
+ "ofs://host:9862/, host, 9862",
+ "ofs://omservice1/, omservice1, -1",
+ // service id with an underscore: HostAndPort tolerates it (URI.getHost
does not)
+ "ofs://om_service/, om_service, -1",
+ // IPv6 literal authority, with and without a port; the literal stays
bracketed
+ "ofs://[::1]:9862/, [::1], 9862",
+ "ofs://[2001:db8::1]/, [2001:db8::1], -1",
+ })
Review Comment:
The PR description calls out both `ofs://...` and `o3fs://...` failing for
IPv6 authorities, but the added parameterized cases only cover `ofs://`.
Consider adding equivalent `o3fs://...` cases (and/or a companion test that
initializes `BasicOzoneFileSystem`) so the IPv6 authority fix is exercised for
both schemes.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -158,21 +159,20 @@ public void initialize(URI name, Configuration conf)
throws IOException {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
- String omHostOrServiceId;
- int omPort = -1;
- // Parse hostname and port
- String[] parts = authority.split(":");
- if (parts.length > 2) {
+ // Parse hostname and port. HostAndPort is bracket-aware, so IPv6 literal
+ // authorities (for example [::1]:9862) are split correctly instead of on
+ // every colon.
+ final HostAndPort hostAndPort;
+ try {
+ hostAndPort = HostAndPort.fromString(authority);
+ } catch (IllegalArgumentException e) {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
Review Comment:
The catch block rethrows a new `IllegalArgumentException` without preserving
the original exception as the cause, which makes debugging malformed
authorities harder. Consider chaining the original exception (eg `new
IllegalArgumentException(URI_EXCEPTION_TEXT, e)`) while keeping the same
user-facing message.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -158,21 +159,20 @@ public void initialize(URI name, Configuration conf)
throws IOException {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
- String omHostOrServiceId;
- int omPort = -1;
- // Parse hostname and port
- String[] parts = authority.split(":");
- if (parts.length > 2) {
+ // Parse hostname and port. HostAndPort is bracket-aware, so IPv6 literal
+ // authorities (for example [::1]:9862) are split correctly instead of on
+ // every colon.
+ final HostAndPort hostAndPort;
+ try {
+ hostAndPort = HostAndPort.fromString(authority);
+ } catch (IllegalArgumentException e) {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
- omHostOrServiceId = parts[0];
- if (parts.length == 2) {
- try {
- omPort = Integer.parseInt(parts[1]);
- } catch (NumberFormatException e) {
- throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
- }
- }
+ int omPort = hostAndPort.hasPort() ? hostAndPort.getPort() : -1;
Review Comment:
`HostAndPort` enforces stricter port validation than the previous
`Integer.parseInt(...)` approach (eg rejecting negative ports and ports outside
0-65535). If that stricter validation is intended, it would be good to add an
explicit test case documenting the expected failure mode/message for invalid
ports; if not intended, consider aligning behavior with the previous parsing
logic.
##########
hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneFileSystem.java:
##########
@@ -158,21 +159,20 @@ public void initialize(URI name, Configuration conf)
throws IOException {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
- String omHostOrServiceId;
- int omPort = -1;
- // Parse hostname and port
- String[] parts = authority.split(":");
- if (parts.length > 2) {
+ // Parse hostname and port. HostAndPort is bracket-aware, so IPv6 literal
+ // authorities (for example [::1]:9862) are split correctly instead of on
+ // every colon.
+ final HostAndPort hostAndPort;
+ try {
+ hostAndPort = HostAndPort.fromString(authority);
+ } catch (IllegalArgumentException e) {
throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
}
- omHostOrServiceId = parts[0];
- if (parts.length == 2) {
- try {
- omPort = Integer.parseInt(parts[1]);
- } catch (NumberFormatException e) {
- throw new IllegalArgumentException(URI_EXCEPTION_TEXT);
- }
- }
+ int omPort = hostAndPort.hasPort() ? hostAndPort.getPort() : -1;
+ String host = hostAndPort.getHost();
+ // Keep IPv6 literals bracketed so the downstream host:port assembly that
+ // builds the OM address stays unambiguous.
+ String omHostOrServiceId = host.contains(":") ? "[" + host + "]" : host;
Review Comment:
Manually re-bracketing IPv6 literals here bakes formatting concerns into
`omHostOrServiceId`, which may later be used as a hostname (where brackets are
often not expected) vs an authority string (where brackets are required). A
more maintainable approach is to keep `host` unbracketed as the canonical host
value, and only apply bracket-aware formatting at the point where a
`host:port`/authority string is constructed (eg via an existing helper like
`getHostPortString`).
--
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]
