[GitHub] [accumulo] brianloss commented on a change in pull request #2551: Add compareTo method to HostAndPort class

[GitBox <gi...@apache.org>]

brianloss commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r821993295



##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -281,4 +282,13 @@ public int getPortOrDefault(int defaultPort) {
     return hasPort() ? port : defaultPort;
   }
 
+  /**
+   * HostAndPort must implement compareTo. As this is a seldom used utiltiy, compareTo simply treats

Review comment:
       Typo...
   ```suggestion
      * HostAndPort must implement compareTo. As this is a seldom used utility, compareTo simply treats
   ```

##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -281,4 +282,13 @@ public int getPortOrDefault(int defaultPort) {
     return hasPort() ? port : defaultPort;
   }
 
+  /**
+   * HostAndPort must implement compareTo. As this is a seldom used utiltiy, compareTo simply treats
+   * HostAndPort values as Strings.
+   */
+  @Override
+  public int compareTo(HostAndPort other) {
+    return Comparator.nullsFirst(Comparator.comparing(HostAndPort::toString)).compare(this, other);

Review comment:
       It doesn't look like the port value is padded, so using the toString value won't necessarily compare properly if the host is the same and the port differs in the number of digits (e.g., when using multiple tservers per host and os-selected tserver ports, this could happen depending on the OS configuration).




-- 
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: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org