danielhumanmod commented on code in PR #516:
URL: https://github.com/apache/polaris/pull/516#discussion_r1984474418
##########
service/common/src/main/java/org/apache/polaris/service/task/ManifestFileCleanupTaskHandler.java:
##########
@@ -42,50 +40,32 @@
import org.slf4j.LoggerFactory;
/**
- * {@link TaskHandler} responsible for deleting table files: 1. Manifest
files: It contains all the
- * files in a manifest and the manifest itself. Since data files may be
present in multiple
- * manifests across different snapshots, we assume a data file that doesn't
exist is missing because
- * it was already deleted by another task. 2. Table metadata files: It
contains previous metadata
- * and statistics files, which are grouped and deleted in batch
+ * {@link ManifestFileCleanupTaskHandler} responsible for deleting all the
files in a manifest and
+ * the manifest itself. Since data files may be present in multiple manifests
across different
+ * snapshots, we assume a data file that doesn't exist is missing because it
was already deleted by
+ * another task.
*/
-// TODO: Rename this class since we introducing metadata cleanup here
-public class ManifestFileCleanupTaskHandler implements TaskHandler {
- public static final int MAX_ATTEMPTS = 3;
- public static final int FILE_DELETION_RETRY_MILLIS = 100;
+public class ManifestFileCleanupTaskHandler extends FileCleanupTaskHandler {
private static final Logger LOGGER =
LoggerFactory.getLogger(ManifestFileCleanupTaskHandler.class);
- private final TaskFileIOSupplier fileIOSupplier;
- private final ExecutorService executorService;
public ManifestFileCleanupTaskHandler(
TaskFileIOSupplier fileIOSupplier, ExecutorService executorService) {
- this.fileIOSupplier = fileIOSupplier;
- this.executorService = executorService;
+ super(fileIOSupplier, executorService);
}
@Override
public boolean canHandleTask(TaskEntity task) {
- return task.getTaskType() == AsyncTaskType.MANIFEST_FILE_CLEANUP
- || task.getTaskType() == AsyncTaskType.METADATA_FILE_BATCH_CLEANUP;
+ return task.getTaskType() == AsyncTaskType.MANIFEST_FILE_CLEANUP;
}
@Override
public boolean handleTask(TaskEntity task, CallContext callContext) {
ManifestCleanupTask cleanupTask = task.readData(ManifestCleanupTask.class);
- TableIdentifier tableId = cleanupTask.getTableId();
+ TableIdentifier tableId = cleanupTask.tableId();
Review Comment:
Quote a prev comment:
> [adutra](https://github.com/adutra) [on Dec 11,
2024](https://github.com/apache/polaris/pull/516#discussion_r1879683280)
Can this be a record?
Here we take Alexandre's suggestion to make the `ManifestCleanupTask` a
record as it can eliminate redundant code by automatically generating
constructors, getters, equals(), hashCode(), and toString(), while enforcing
immutability.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
