[GitHub] [accumulo] ctubbsii commented on a diff in pull request #2792: closes #1377 - ensure all tables are checked ...

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

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


##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java:
##########
@@ -72,6 +72,17 @@ public interface GarbageCollectionEnvironment {
    */
   Stream<Reference> getReferences();
 
+  /**
+   * Return a list of all TableIDs in the

Review Comment:
   It doesn't return a list. It returns a set.



##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java:
##########
@@ -72,6 +72,17 @@ public interface GarbageCollectionEnvironment {
    */
   Stream<Reference> getReferences();
 
+  /**
+   * Return a list of all TableIDs in the
+   * {@link org.apache.accumulo.core.metadata.schema.Ample.DataLevel} for which we are considering
+   * deletes. When operating on DataLevel.USER this will return all user table ids. When operating
+   * on DataLevel.METADATA this will return the table id for the accumulo.metadata table. When
+   * operating on DataLevel.ROOT this will return the table id for the accumulo.root table.
+   *
+   * @return The table ids
+   */
+  Set<TableId> getCandidateTableIDs();

Review Comment:
   The term "operating on" used throughout this javadoc is very confusing. What does it mean to be "operating on" a particular level? I think we could choose a better verb to understand what's going on. Maybe "when gathering file candidates for deletion for DataLevel.USER, this will return a list of user table IDs".



##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -216,6 +225,46 @@ private long removeBlipCandidates(GarbageCollectionEnvironment gce,
     return blipCount;
   }
 
+  @VisibleForTesting
+  /**
+   * Double check no tables were missed during GC
+   */
+  protected void ensureAllTablesChecked(Set<TableId> tableIdsBefore, Set<TableId> tableIdsSeen,
+      Set<TableId> tableIdsAfter) {
+
+    // if a table was added or deleted during this run, it is acceptable to not
+    // have seen those tables ids when scanning the metadata table. So get the intersection
+    final Set<TableId> tableIdsMustHaveSeen = new HashSet<>(tableIdsBefore);
+    tableIdsMustHaveSeen.retainAll(tableIdsAfter);
+
+    if (tableIdsMustHaveSeen.isEmpty() && !tableIdsSeen.isEmpty()) {
+      throw new RuntimeException("Garbage collection will not proceed because "
+          + "table ids were seen in the metadata table and none were seen Zookeeper. "
+          + "This can have two causes. First, total number of tables going to/from "
+          + "zero during a GC cycle will cause this. Second, it could be caused by "
+          + "corruption of the metadata table and/or Zookeeper. Only the second cause "
+          + "is problematic, but there is no way to distinguish between the two causes "
+          + "so this GC cycle will not proceed. The first cause should be transient "
+          + "and one would not expect to see this message repeated in subsequent GC cycles.");
+    }
+
+    // From that intersection, remove all the table ids that were seen.
+    tableIdsMustHaveSeen.removeAll(tableIdsSeen);
+
+    // If anything is left then we missed a table and may not have removed rfiles references
+    // from the candidates list that are acutally still in use, which would
+    // result in the rfiles being deleted in the next step of the GC process
+    if (!tableIdsMustHaveSeen.isEmpty()) {
+      log.error("TableIDs before: " + tableIdsBefore);
+      log.error("TableIDs after : " + tableIdsAfter);
+      log.error("TableIDs seen  : " + tableIdsSeen);
+      log.error("TableIDs that should have been seen but were not: " + tableIdsMustHaveSeen);
+      // maybe a scan failed?
+      throw new RuntimeException(
+          "Saw table IDs in ZK that were not in metadata table:  " + tableIdsMustHaveSeen);
+    }

Review Comment:
   Please don't introduce new code with generic RTE being thrown. A more specific RTE is always available and likely more appropriate.
   
   Also, this situation is likely to be common if creating and deleting a lot of tables. The canonical determination of whether a table exists or not is that it has an entry in ZK... this is created before metadata entries, and is the last thing removed when a table is deleted. So, it's quite normal and routine for a table ID not to exist in the metadata table if it was just created or deleted. This is not necessarily a problem and shouldn't throw an exception... or we're going to see this a lot. We can probably start over gathering candidates... but we shouldn't just crash.



##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -462,4 +479,20 @@ public long getErrorsStat() {
   public long getCandidatesStat() {
     return candidates;
   }
+
+  @Override
+  public Set<TableId> getCandidateTableIDs() {
+    if (level == DataLevel.ROOT) {
+      return Set.of(RootTable.ID);
+    } else if (level == DataLevel.METADATA) {
+      return Collections.singleton(MetadataTable.ID);
+    } else if (level == DataLevel.USER) {
+      Set<TableId> tableIds = new HashSet<>(getTableIDs());
+      tableIds.remove(MetadataTable.ID);
+      tableIds.remove(RootTable.ID);
+      return tableIds;
+    } else {
+      throw new RuntimeException("Unexpected Table in GC Env: " + this.level.name());

Review Comment:
   Choose more specific RTE



##########
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java:
##########
@@ -135,10 +137,14 @@ private SortedMap<String,String> makeRelative(Collection<String> candidates) {
 
   private void removeCandidatesInUse(GarbageCollectionEnvironment gce,
       SortedMap<String,String> candidateMap) {
+
+    Set<TableId> tableIdsBefore = gce.getCandidateTableIDs();
+    Set<TableId> tableIdsSeen = new HashSet<>();
     var refStream = gce.getReferences();
     Iterator<Reference> iter = refStream.iterator();
     while (iter.hasNext()) {
       Reference ref = iter.next();
+      tableIdsSeen.add(ref.getTableId());

Review Comment:
   This `var` obscures the type and variable name `refStream` implies it's a Stream. But, then we call `.iterator()` on it. If it's actually a Stream, then there are better ways to manipulate it than doing `while (iter.hasNext()) { iter.next(); }`.



##########
server/gc/src/main/java/org/apache/accumulo/gc/GCRun.java:
##########
@@ -172,7 +177,19 @@ public Stream<Reference> getReferences() {
 
   @Override
   public Set<TableId> getTableIDs() {
-    return context.getTableIdToNameMap().keySet();
+    final String tablesPath = context.getZooKeeperRoot() + Constants.ZTABLES;
+    final ZooReader zr = context.getZooReader();
+    final Set<TableId> tids = new HashSet<>();
+    while (true) {
+      try {
+        zr.sync(tablesPath);
+        zr.getChildren(tablesPath).forEach(t -> tids.add(TableId.of(t)));
+        return tids;
+      } catch (KeeperException | InterruptedException e) {
+        log.error("Error getting tables from ZooKeeper, retrying", e);
+        UtilWaitThread.sleepUninterruptibly(1, TimeUnit.SECONDS);
+      }

Review Comment:
   First, this shouldn't loop if it's explicitly interrupted. The process should probably die in that case. Second, if this does loop because of a KeeperException, the set of table IDs that was constructed prior to the `while (true)` loop could include deleted tables. Is that okay/intended? If so, a small inline comment explaining why that's okay might be useful for future onlookers.



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