Copilot commented on code in PR #8413:
URL: https://github.com/apache/hbase/pull/8413#discussion_r3470839030
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/Strings.java:
##########
@@ -28,6 +28,7 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.hbase.thirdparty.com.google.common.net.InetAddresses;
import org.apache.hbase.thirdparty.com.google.common.base.Joiner;
import org.apache.hbase.thirdparty.com.google.common.base.Splitter;
Review Comment:
The newly added third-party imports are not in the usual alphabetical order
(e.g., compare ServerName.java where com.google.common.base.* appears before
com.google.common.net.*). This can trip checkstyle/import-order rules.
##########
hbase-common/src/main/java/org/apache/hadoop/hbase/util/Strings.java:
##########
@@ -88,6 +89,26 @@ public static String domainNamePointerToHostName(String
dnPtr) {
return dnPtr.endsWith(".") ? dnPtr.substring(0, dnPtr.length() - 1) :
dnPtr;
}
+ /**
+ * Compare two host identifiers for equality. DNS hostnames are compared
case-insensitively because
+ * DNS labels are case-insensitive. IP address literals are compared exactly.
+ * @param left first hostname or IP literal
+ * @param right second hostname or IP literal
+ * @return {@code true} if both refer to the same host identifier
+ */
+ public static boolean hostnamesEqual(String left, String right) {
+ if (left == right) {
+ return true;
+ }
+ if (left == null || right == null) {
+ return false;
+ }
+ if (InetAddresses.isInetAddress(left) ||
InetAddresses.isInetAddress(right)) {
+ return left.equals(right);
+ }
+ return left.equalsIgnoreCase(right);
+ }
Review Comment:
hostnamesEqual compares IP address literals using raw String equality when
either side looks like an IP. This will return false for equivalent IPv6
literals with different casing (hex digits) or different valid textual forms
(compressed vs expanded), potentially causing false startup failures. Consider
parsing and comparing numeric InetAddress values when both sides are IP
literals, and returning false when only one side is an IP literal.
##########
hbase-common/src/test/java/org/apache/hadoop/hbase/util/TestStrings.java:
##########
@@ -57,6 +58,18 @@ public void testDomainNamePointerToHostName() {
assertEquals("foo.bar.com",
Strings.domainNamePointerToHostName("foo.bar.com."));
}
+ @Test
+ public void testHostnamesEqual() {
+ assertTrue(Strings.hostnamesEqual(null, null));
+ assertFalse(Strings.hostnamesEqual("host.example.com", null));
+ assertTrue(Strings.hostnamesEqual("HOST.example.com", "host.example.com"));
+ assertTrue(Strings.hostnamesEqual("rs1", "RS1"));
+ assertFalse(Strings.hostnamesEqual("host-a.example.com",
"host-b.example.com"));
+ assertTrue(Strings.hostnamesEqual("10.0.0.1", "10.0.0.1"));
+ assertFalse(Strings.hostnamesEqual("10.0.0.1", "10.0.0.2"));
+ assertFalse(Strings.hostnamesEqual("HOST.example.com", "10.0.0.1"));
+ }
Review Comment:
The new tests cover case-insensitive DNS hostnames and IPv4 literals, but
they do not cover IPv6 where textual representations can differ (hex casing,
compressed vs expanded) even when the numeric address is the same. Adding a
couple of IPv6 assertions would prevent regressions once hostnamesEqual is used
for RS startup checks.
--
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]
