[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3703: add check to gc to validate metadata scan is complete

["ctubbsii (via GitHub)" <gi...@apache.org>]

ctubbsii commented on code in PR #3703:
URL: https://github.com/apache/accumulo/pull/3703#discussion_r1297940235


##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -343,4 +358,63 @@ public void collect(GarbageCollectionEnvironment gce)
       deleteConfirmed(gce, candidateMap);
     }
   }
+
+  /**
+   * Track metadata rows read to help validate that gc scan has complete information to make a
+   * decision on deleting files
+   */
+  private static class MetadataReadTracker {
+    private boolean hasDir = false;
+    private boolean hasPrevRow = false;
+    private Text row;
+
+    private boolean closed = false;
+
+    public MetadataReadTracker() {
+      this.row = null;
+    }
+
+    private void checkRow() {
+      Preconditions.checkState(hasDir && hasPrevRow,
+          "May not have fully read metadata for row, aborting this run. Validation results: %s",
+          this);
+    }
+
+    public void sawRow(final Text candidate) {
+      Preconditions.checkState(!closed);
+      Objects.requireNonNull(candidate);
+      if (row == null) {
+        row = candidate;
+      } else if (!row.equals(candidate)) {
+        checkRow();
+        hasPrevRow = false;
+        hasDir = false;
+        row = candidate;
+      }
+    }
+
+    public void sawDir() {
+      Preconditions.checkState(!closed);
+      hasDir = true;
+    }
+
+    public void sawPrevRow() {
+      Preconditions.checkState(!closed);
+      hasPrevRow = true;
+    }
+
+    public void sawLastRow() {
+      Preconditions.checkState(!closed);
+      if (row != null) {
+        checkRow();
+      }
+      closed = true;
+    }
+
+    @Override
+    public String toString() {
+      return "MetadataReadCheck{row=" + row + ", hasDir=" + hasDir + ", hasPrevRow=" + hasPrevRow
+          + '}';
+    }

Review Comment:
   The name here no longer matches the name of the class. Guava has a nice "ToStringHelper" class. I'm not sure if it's worth adding it here. Adding in more Guava might just be a pain later.
   
   ```suggestion
       public String toString() {
         return getClass().getSimpleName() + "{row=" + row + ", hasDir=" + hasDir + ", hasPrevRow=" + hasPrevRow
             + '}';
       }
   ```
   (Note: formatting is almost certainly off in this suggestion.



##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -211,12 +221,17 @@ private void confirmDeletes(GarbageCollectionEnvironment gce,
 
         dir = makeRelative(dir, 2);
 
-        if (candidateMap.remove(dir) != null)
+        if (candidateMap.remove(dir) != null) {
           log.debug("Candidate was still in use: " + dir);
+        }
+      } else if (TabletsSection.TabletColumnFamily.PREV_ROW_COLUMN.hasColumns(key)) {
+        readTracker.sawPrevRow();
       } else
         throw new RuntimeException(
             "Scanner over metadata table returned unexpected column : " + entry.getKey());
     }
+    // process last row to check metadata read included the dir entry.
+    readTracker.sawLastRow();

Review Comment:
   The name of this method is a bit confusing. It implies positive affirmation that the last row possible was actually seen. But that's what it's supposed to be verifying. So, maybe rename it like:
   
   ```suggestion
       readTracker.checkLastRow();
   ```
   
   Or just call it `close` and make it AutoCloseable and use it with try-with-resources.



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