[GitHub] [iceberg] rdblue commented on a change in pull request #1187: Add support to parallelise the execution of tasks of metadata and data files delete operations

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

rdblue commented on a change in pull request #1187:
URL: https://github.com/apache/iceberg/pull/1187#discussion_r453929734



##########
File path: api/src/main/java/org/apache/iceberg/ExpireSnapshots.java
##########
@@ -82,4 +83,18 @@
    * @return this for method chaining
    */
   ExpireSnapshots deleteWith(Consumer<String> deleteFunc);
+
+  /**
+   * Passes an alternative executor service that will be used for manifests and data files deletion.
+   * <p>
+   * Manifest files that are no longer used by valid snapshots will be deleted. Data files that were
+   * deleted by snapshots that are expired will be deleted.
+   * <p>
+   * If this method is not called, unnecessary manifests and data files will still be deleted using a single threaded
+   * executor service.
+   *
+   * @param executorService an executor service to parallelize tasks to delete manifests and data files
+   * @return this for method chaining
+   */
+  ExpireSnapshots deleteWith(ExecutorService executorService);

Review comment:
       Because there is already a `deleteWith` method that controls an option that is substantially different, I don't think it is a good idea to name the `deleteWith`. It seems odd to pass a callback to one and an executor service to another. What about naming this `executeWith` instead?




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

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org