[GitHub] [accumulo] dlmarion commented on a diff in pull request #2750: Replace String with Reference in GC & Ample

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

dlmarion commented on code in PR #2750:
URL: https://github.com/apache/accumulo/pull/2750#discussion_r888145427


##########
core/src/main/java/org/apache/accumulo/core/metadata/TabletFileUtil.java:
##########
@@ -36,6 +36,14 @@ public static String validate(String path) {
     return validate(p).toString();
   }
 
+  public static Reference validate(Reference reference) {
+    Path p = new Path(reference.metadataEntry);
+    if (p.toUri().getScheme() == null) {
+      throw new IllegalArgumentException("Invalid path provided, no scheme in " + reference);
+    }
+    return reference;

Review Comment:
   ```suggestion
       validate(new Path(reference.metadataEntry));
       return reference;
   ```



##########
server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java:
##########
@@ -635,31 +637,34 @@ private void deleteTablets(MergeInfo info) throws AccumuloException {
       ServerColumnFamily.TIME_COLUMN.fetch(scanner);
       scanner.fetchColumnFamily(DataFileColumnFamily.NAME);
       scanner.fetchColumnFamily(CurrentLocationColumnFamily.NAME);
-      Set<String> datafiles = new TreeSet<>();
+      Set<Reference> datafilesAndDirs = new TreeSet<>();

Review Comment:
   I think Reference and maybe it's subclasses are going to need a Comparator or implement Comparable to maintain the same order. It may be useful to also have them implement `equals` and `hashCode` too.



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