[GitHub] [accumulo] jmark99 commented on a change in pull request #2214: Remove continue point from Garbage Collector

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

jmark99 commented on a change in pull request #2214:
URL: https://github.com/apache/accumulo/pull/2214#discussion_r677482789



##########
File path: server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionEnvironment.java
##########
@@ -40,19 +39,10 @@
 public interface GarbageCollectionEnvironment {
 
   /**
-   * Return a list of paths to files and dirs which are candidates for deletion from a given table,
-   * {@link RootTable#NAME} or {@link MetadataTable#NAME}
-   *
-   * @param continuePoint
-   *          A row to resume from if a previous invocation was stopped due to finding an extremely
-   *          large number of candidates to remove which would have exceeded memory limitations
-   * @param candidates
-   *          A collection of candidates files for deletion, may not be the complete collection of
-   *          files for deletion at this point in time
-   * @return true if the results are short due to insufficient memory, otherwise false
+   * Process all possible deletion candidates for a given table, deleting candidates that meet all
+   * necessary conditions.
    */
-  boolean getCandidates(String continuePoint, List<String> candidates)
-      throws TableNotFoundException;
+  void processCandidates() throws TableNotFoundException, IOException;

Review comment:
       Given that the processCandidates method is only about three lines, not counting the loop, I'm not sure it is worth it. I could re-create a method, getCandidates, that would be a one-liner returning the required Iterator and then gather the candidates and process them separately.
   
   In the current code using a continue point, the getCandidates method  does get potential candidates and then returns to the collect method to do the processing. With the updated code using the iterator there is less of a separation between those two actions. Passing the iterator around between classes could become confusing as well. I had tried to do something similar to what you are suggesting but I was having issues with the iterator not keeping track of the candidates properly after being passed between multiple methods and classes.  Most likely coder error, but I eventually caved and went back to the current form that you see now.  I could try again if you feel strongly about it.




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