[GitHub] [accumulo] ctubbsii commented on a change in pull request #1774: Refactor Tablet state classes

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

ctubbsii commented on a change in pull request #1774:
URL: https://github.com/apache/accumulo/pull/1774#discussion_r521069066



##########
File path: core/src/test/java/org/apache/accumulo/core/metadata/schema/TabletMetadataTest.java
##########
@@ -145,7 +145,7 @@ public void testFuture() {
         EnumSet.allOf(ColumnType.class), false);
 
     assertEquals(extent, tm.getExtent());
-    assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getHostAndPort());
+    assertEquals(HostAndPort.fromParts("server1", 8555), tm.getLocation().getLocation());

Review comment:
       `getLocation().getLocation()` seems funny here. Too many types known as 'location'.

##########
File path: core/src/main/java/org/apache/accumulo/core/clientImpl/TableOperationsImpl.java
##########
@@ -1301,7 +1301,7 @@ private void waitForTableStateTransition(TableId tableId, TableState expectedSta
           lastRow = tablet.getExtent().toMetaRow();
 
           if (loc != null) {
-            serverCounts.increment(loc.getId(), 1);
+            serverCounts.increment(loc.toString(), 1);

Review comment:
       This seems like a regression. In general, unless you're printing something, it's better to use a more specific type than a String type, and a more specific method name than `toString()`, since `toString()` should not be relied upon to not change over time.




----------------------------------------------------------------
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.

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