Re: [PR] refactored findTabletsNeedingAttention test to handle new return type. [accumulo]

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on code in PR #4418:
URL: https://github.com/apache/accumulo/pull/4418#discussion_r1546813163


##########
test/src/main/java/org/apache/accumulo/test/functional/TabletManagementIteratorIT.java:
##########
@@ -165,72 +177,92 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // t3 is hosted, setting to never will generate a change to unhost tablets
       setTabletAvailability(client, metaCopy1, t3, TabletAvailability.UNHOSTED.name());
       tabletMgmtParams = createParameters(client);
-      assertEquals(4, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams),
+      assertEquals(4, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams).size(),

Review Comment:
   Should be able to create the map here instead of using 4.  The tablets for tables t1 and t3 will need location update.  Maybe something like the following.
   
   ```suggestion
         assertEquals(Map.of(endR1, Set.of(NEEDS_LOCATION_UPDATE), prevR1, Set.of(NEEDS_LOCATION_UPDATE), endR3, Set.of(NEEDS_LOCATION_UPDATE), prevR3, Set.of(NEEDS_LOCATION_UPDATE)), findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams),
             "Should have four tablets with hosting availability changes");
   ```



##########
test/src/main/java/org/apache/accumulo/test/functional/TabletManagementIteratorIT.java:
##########
@@ -141,15 +142,26 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // examine a clone of the metadata table, so we can manipulate it
       copyTable(client, AccumuloTable.METADATA.tableName(), metaCopy1);
 
+      // Create expected KeyExtents to test output of findTabletsNeedingAttention
+      KeyExtent endR1 = new KeyExtent(TableId.of("1"), new Text("some split"), null);
+      KeyExtent endR3 = new KeyExtent(TableId.of("3"), new Text("some split"), null);
+      KeyExtent endR4 = new KeyExtent(TableId.of("4"), new Text("some split"), null);
+      KeyExtent prevR1 = new KeyExtent(TableId.of("1"), null, new Text("some split"));
+      KeyExtent prevR3 = new KeyExtent(TableId.of("3"), null, new Text("some split"));
+      KeyExtent prevR4 = new KeyExtent(TableId.of("4"), null, new Text("some split"));

Review Comment:
   Table ids may change, good to resolve them at runtime.  Could do something like the following.
   
   ```suggestion
         var tableId1 = getServerContext().getTableId(t1);
         var tableId3 = getServerContext().getTableId(t3);
         var tableId4 = getServerContext().getTableId(t4);
   
         // Create expected KeyExtents to test output of findTabletsNeedingAttention
         KeyExtent endR1 = new KeyExtent(tableId1, new Text("some split"), null);
         KeyExtent endR3 = new KeyExtent(tableId3, new Text("some split"), null);
         KeyExtent endR4 = new KeyExtent(tableId4, new Text("some split"), null);
         KeyExtent prevR1 = new KeyExtent(tableId1, null, new Text("some split"));
         KeyExtent prevR3 = new KeyExtent(tableId3, null, new Text("some split"));
         KeyExtent prevR4 = new KeyExtent(tableId4, null, new Text("some split"));
   ```



##########
test/src/main/java/org/apache/accumulo/test/functional/TabletManagementIteratorIT.java:
##########
@@ -165,72 +177,92 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // t3 is hosted, setting to never will generate a change to unhost tablets
       setTabletAvailability(client, metaCopy1, t3, TabletAvailability.UNHOSTED.name());
       tabletMgmtParams = createParameters(client);
-      assertEquals(4, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams),
+      assertEquals(4, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams).size(),
           "Should have four tablets with hosting availability changes");
 
       // test the assigned case (no location)
       removeLocation(client, metaCopy1, t3);
-      assertEquals(2, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams),
+      expected = Map.of(endR1, Set.of(TabletManagement.ManagementAction.NEEDS_LOCATION_UPDATE),

Review Comment:
   Statically importing `TabletManagement.ManagementAction.NEEDS_LOCATION_UPDATE` would make the test code more concise because could then write `Set.of(NEEDS_LOCATION_UPDATE)`



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