[GitHub] [accumulo] ctubbsii commented on a change in pull request #2309: fixes #2111 adds unit test for external compaction metadata validation

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

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



##########
File path: server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/CompactableImpl.java
##########
@@ -815,16 +812,47 @@ private void checkIfUserCompactionCanceled() {
   }
 
   /**
-   * For user compactions a set of files is selected. Those files then get compacted by one or more
-   * compactions until the set is empty. This method attempts to reconstruct the selected set of
-   * files when a tablet is loaded with an external user compaction. It avoids repeating work and
-   * when a user compaction completes, files are verified against the selected set. Since the data
-   * is coming from persisted storage, lots of checks are done in this method rather than assuming
-   * the persisted data is correct.
+   * This method sanity checks metadata about external compactions. It also extracts specific
+   * information needed for user and selector compactions.
    */
-  private Optional<SelectedInfo> initExternalSelection(
-      Map<ExternalCompactionId,ExternalCompactionMetadata> extCompactions, Tablet tablet,
+  static Optional<SelectedInfo> processExternalMetadata(
+      Map<ExternalCompactionId,ExternalCompactionMetadata> extCompactions,
+      Supplier<Optional<Long>> tabletCompactionId, Set<StoredTabletFile> tabletFiles,
       Map<ExternalCompactionId,String> externalCompactionsToRemove) {
+
+    // Check that external compactions have disjoint sets of files. Also check that each external
+    // compaction only has files inside a tablet.
+    Set<StoredTabletFile> seen = new HashSet<>();
+    boolean overlap = false;
+
+    for (var entry : extCompactions.entrySet()) {
+      ExternalCompactionMetadata ecMeta = entry.getValue();
+      if (!tabletFiles.containsAll(ecMeta.getJobFiles())) {
+        externalCompactionsToRemove.putIfAbsent(entry.getKey(),
+            "Has files outside of tablet files");
+      } else if (!Collections.disjoint(seen, ecMeta.getJobFiles())) {
+        overlap = true;
+      }
+      seen.addAll(ecMeta.getJobFiles());
+    }
+
+    if (overlap) {
+      extCompactions.keySet().forEach(ecid -> {
+        externalCompactionsToRemove.putIfAbsent(ecid, "Some external compaction files overlap");
+      });
+      return Optional.empty();
+    }
+
+    /*
+     * The rest of the code sanity checks user compaction metadata and extracts needed information.
+     * 

Review comment:
       ```suggestion
        * <p>
   ```




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