You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/03/08 15:10:58 UTC
[GitHub] [accumulo] jmark99 opened a new pull request #2551: Add compareTo method to HostAndPort class
jmark99 opened a new pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551
The utility class, VerifyTabletAssignements, uses a TreeMap collection
to hold HostAndPort values. This requires the HostAndPort class to
implement the Comparable interface. This ticket adds the compareTo
method to HostAndPort. Given that the utility is seldom used, the
compareTo uses a simple compare on the Host portion and an integer
compare on the Port if required.
A HostAndPortTest class was added to verify changes.
This was first found when running ErrorProne. The ErrorProne check for
this condition had been disabled due to this issue. The check has now
been re-enabled in the ErrorProne configuration within the POM file.
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822894763
##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -281,4 +282,16 @@ public int getPortOrDefault(int defaultPort) {
return hasPort() ? port : defaultPort;
}
+ /**
+ * HostAndPort must implement compareTo. As this is a seldom used utility, compareTo simply treats
+ * HostAndPort values as Strings.
+ */
+ @Override
+ public int compareTo(HostAndPort other) {
+ return Comparator
+ .nullsFirst(
+ Comparator.comparing(HostAndPort::getHost).thenComparingInt(h -> h.getPortOrDefault(0)))
Review comment:
Does this still work if host is null? From the constructor, it doesn't look like we require non-null hosts. This might fail to compare if the host is null.
--
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
[GitHub] [accumulo] brianloss commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
brianloss commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822036320
##########
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:
The case I'm thinking of is where a host, say localhost, has ports 9999 and 10000. The 10000 will sort first if you use a string comparison from toString, and second if you were actually comparing the integer port values.
--
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
[GitHub] [accumulo] jmark99 merged pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
jmark99 merged pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822044449
##########
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:
See the comment I made above regarding `thenComparingInt` for comparing the port.
--
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
[GitHub] [accumulo] jmark99 commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822644616
##########
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:
@brianloss modified the comparator to treat port values as ints and added test cases to verify.
--
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
[GitHub] [accumulo] brianloss commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by 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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r821784698
##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -281,4 +281,18 @@ public int getPortOrDefault(int defaultPort) {
return hasPort() ? port : defaultPort;
}
+ /**
+ * HostAndPort must implement compareTo. As this is a seldom used utiltiy, compareTo simply orders
+ * HostAndPort values using a String compare on the Host value with a secondary integer compare on
+ * the Port if Host values are identical.
+ */
+ @Override
+ public int compareTo(HostAndPort other) {
+ if (this == other) {
+ return 0;
+ }
+ int hostCompareValue = this.getHost().compareTo(other.getHost());
Review comment:
host can be null
--
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
[GitHub] [accumulo] ctubbsii commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
ctubbsii commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822907068
##########
File path: core/src/main/java/org/apache/accumulo/core/util/HostAndPort.java
##########
@@ -281,4 +282,16 @@ public int getPortOrDefault(int defaultPort) {
return hasPort() ? port : defaultPort;
}
+ /**
+ * HostAndPort must implement compareTo. As this is a seldom used utility, compareTo simply treats
+ * HostAndPort values as Strings.
+ */
+ @Override
+ public int compareTo(HostAndPort other) {
+ return Comparator
+ .nullsFirst(
+ Comparator.comparing(HostAndPort::getHost).thenComparingInt(h -> h.getPortOrDefault(0)))
Review comment:
Oh, and I forgot to mention. Rather than constructing a new comparator each time, you can save the comparator in a static private final field, and just call it in the compareTo method.
--
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
[GitHub] [accumulo] jmark99 commented on a change in pull request #2551: Add compareTo method to HostAndPort class
Posted by GitBox <gi...@apache.org>.
jmark99 commented on a change in pull request #2551:
URL: https://github.com/apache/accumulo/pull/2551#discussion_r822034570
##########
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:
I added a couple of new test cases to verify that the HostAndPort class handles padded port values.
--
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