fixes continue scan in tablet mgmt iterator (apache#4457)

Many places in the accumulo code will read a batch of key/values and
then use the last key in the batch to construct a range to get the next
batch.  The last key in the batch will be a non inclusive start key for
the range.  The tablet mgmt iterator was not handling this case and
returning the key that should have been excluded.

Author: keith-turner

server/base/src/main/java/org/apache/accumulo/server/manager/state/TabletManagementIterator.java

@@ -217,7 +217,11 @@ protected void consume() throws IOException {
           // can pull this K,V pair from the results by looking at the colf.
           TabletManagement.addActions(decodedRow, actions);
         }
-        topKey = decodedRow.firstKey();
+
+        // This key is being created exactly the same way as the whole row iterator creates keys.
+        // This is important for ensuring that seek works as expected in the continue case. See
+        // WholeRowIterator seek function for details, it looks for keys w/o columns.
+        topKey = new Key(decodedRow.firstKey().getRow());
         topValue = WholeRowIterator.encodeRow(new ArrayList<>(decodedRow.keySet()),
             new ArrayList<>(decodedRow.values()));
         LOG.trace("Returning extent {} with reasons: {}", tm.getExtent(), actions);

test/src/main/java/org/apache/accumulo/test/functional/TabletManagementIteratorIT.java

@@ -18,8 +18,13 @@
  */
 package org.apache.accumulo.test.functional;
 
+import static org.apache.accumulo.core.manager.state.TabletManagement.ManagementAction.BAD_STATE;
 import static org.apache.accumulo.core.manager.state.TabletManagement.ManagementAction.NEEDS_LOCATION_UPDATE;
+import static org.apache.accumulo.core.manager.state.TabletManagement.ManagementAction.NEEDS_RECOVERY;
+import static org.apache.accumulo.core.manager.state.TabletManagement.ManagementAction.NEEDS_SPLITTING;
+import static org.apache.accumulo.core.manager.state.TabletManagement.ManagementAction.NEEDS_VOLUME_REPLACEMENT;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.io.IOException;
 import java.time.Duration;
@@ -187,6 +192,10 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy1, tabletMgmtParams),
           "Should have four tablets with hosting availability changes");
 
+      // test continue scan functionality, this test needs a table and tablet mgmt params that will
+      // return more than one tablet
+      testContinueScan(client, metaCopy1, tabletMgmtParams);
+
       // test the assigned case (no location)
       removeLocation(client, metaCopy1, t3);
       expected =
@@ -210,18 +219,15 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // Test the recovery cases
       createLogEntry(client, metaCopy5, t1);
       setTabletAvailability(client, metaCopy5, t1, TabletAvailability.UNHOSTED.name());
-      expected = Map.of(endR1,
-          Set.of(NEEDS_LOCATION_UPDATE, TabletManagement.ManagementAction.NEEDS_RECOVERY));
+      expected = Map.of(endR1, Set.of(NEEDS_LOCATION_UPDATE, NEEDS_RECOVERY));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy5, tabletMgmtParams),
           "Only 1 of 2 tablets in table t1 should be returned");
       setTabletAvailability(client, metaCopy5, t1, TabletAvailability.ONDEMAND.name());
-      expected = Map.of(endR1,
-          Set.of(NEEDS_LOCATION_UPDATE, TabletManagement.ManagementAction.NEEDS_RECOVERY));
+      expected = Map.of(endR1, Set.of(NEEDS_LOCATION_UPDATE, NEEDS_RECOVERY));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy5, tabletMgmtParams),
           "Only 1 of 2 tablets in table t1 should be returned");
       setTabletAvailability(client, metaCopy5, t1, TabletAvailability.HOSTED.name());
-      expected = Map.of(endR1,
-          Set.of(NEEDS_LOCATION_UPDATE, TabletManagement.ManagementAction.NEEDS_RECOVERY), prevR1,
+      expected = Map.of(endR1, Set.of(NEEDS_LOCATION_UPDATE, NEEDS_RECOVERY), prevR1,
           Set.of(NEEDS_LOCATION_UPDATE));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy5, tabletMgmtParams),
           "2 tablets in table t1 should be returned");
@@ -248,8 +254,7 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // for both MERGING and SPLITTING
       setOperationId(client, metaCopy4, t4, null, TabletOperationType.MERGING);
       createLogEntry(client, metaCopy4, t4);
-      expected = Map.of(endR4,
-          Set.of(NEEDS_LOCATION_UPDATE, TabletManagement.ManagementAction.NEEDS_RECOVERY));
+      expected = Map.of(endR4, Set.of(NEEDS_LOCATION_UPDATE, NEEDS_RECOVERY));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy4, tabletMgmtParams),
           "Should have a tablet needing attention because of wals");
       // Switch op to SPLITTING which should also need attention like MERGING
@@ -266,8 +271,7 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // test the bad tablet location state case (inconsistent metadata)
       tabletMgmtParams = createParameters(client);
       addDuplicateLocation(client, metaCopy3, t3);
-      expected = Map.of(prevR3,
-          Set.of(NEEDS_LOCATION_UPDATE, TabletManagement.ManagementAction.BAD_STATE));
+      expected = Map.of(prevR3, Set.of(NEEDS_LOCATION_UPDATE, BAD_STATE));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy3, tabletMgmtParams),
           "Should have 1 tablet that needs a metadata repair");
 
@@ -278,7 +282,7 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       Map<Path,Path> replacements =
           Map.of(new Path("file:/vol1/accumulo/inst_id"), new Path("file:/vol2/accumulo/inst_id"));
       tabletMgmtParams = createParameters(client, replacements);
-      expected = Map.of(prevR4, Set.of(TabletManagement.ManagementAction.NEEDS_VOLUME_REPLACEMENT));
+      expected = Map.of(prevR4, Set.of(NEEDS_VOLUME_REPLACEMENT));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy4, tabletMgmtParams),
           "Should have one tablet that needs a volume replacement");
 
@@ -291,7 +295,7 @@ public void test() throws AccumuloException, AccumuloSecurityException, TableExi
       // Lower the split threshold for the table, should cause the files added to need attention.
       client.tableOperations().setProperty(tables[3], Property.TABLE_SPLIT_THRESHOLD.getKey(),
           "1K");
-      expected = Map.of(prevR4, Set.of(TabletManagement.ManagementAction.NEEDS_SPLITTING));
+      expected = Map.of(prevR4, Set.of(NEEDS_SPLITTING));
       assertEquals(expected, findTabletsNeedingAttention(client, metaCopy6, tabletMgmtParams),
           "Should have one tablet that needs splitting");
 
@@ -444,6 +448,29 @@ private Map<KeyExtent,Set<TabletManagement.ManagementAction>> findTabletsNeeding
     return results;
   }
 
+  // Multiple places in the accumulo code will read a batch of keys and then take the last key and
+  // make it non inclusive to get the next batch. This test code simulates that and ensures the
+  // tablet mgmt iterator works with that.
+  private void testContinueScan(AccumuloClient client, String table,
+      TabletManagementParameters tabletMgmtParams) throws TableNotFoundException {
+    try (Scanner scanner = client.createScanner(table, Authorizations.EMPTY)) {
+      TabletManagementIterator.configureScanner(scanner, tabletMgmtParams);
+      List<Entry<Key,Value>> entries1 = new ArrayList<>();
+      scanner.forEach(e -> entries1.add(e));
+      assertTrue(entries1.size() > 1);
+
+      // Create a range that does not include the first key from the last scan.
+      var range = new Range(entries1.get(0).getKey(), false, null, true);
+      scanner.setRange(range);
+
+      // Ensure the first key excluded from the scan
+      List<Entry<Key,Value>> entries2 = new ArrayList<>();
+      scanner.forEach(e -> entries2.add(e));
+      assertEquals(entries1.size() - 1, entries2.size());
+      assertEquals(entries1.get(1).getKey(), entries2.get(0).getKey());
+    }
+  }
+
   private void createTable(AccumuloClient client, String t, boolean online)
       throws AccumuloSecurityException, AccumuloException, TableNotFoundException,
       TableExistsException {