j1wonpark commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r3063067487
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -139,107 +141,163 @@ public void cleanOrphanFiles() {
return;
}
- long keepTime = tableConfiguration.getOrphanExistingMinutes() * 60 * 1000;
+ MaintainerOperationExecutor executor = new
MaintainerOperationExecutor(metrics);
- cleanContentFiles(System.currentTimeMillis() - keepTime, metrics);
+ executor.execute(
+ MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ () -> {
+ long keepTime = tableConfiguration.getOrphanExistingMinutes() * 60 *
1000;
- // refresh
- table.refresh();
+ cleanContentFiles(System.currentTimeMillis() - keepTime, metrics);
- // clear metadata files
- cleanMetadata(System.currentTimeMillis() - keepTime, metrics);
+ // refresh
+ table.refresh();
+
+ // clear metadata files
+ cleanMetadata(System.currentTimeMillis() - keepTime, metrics);
+ });
}
@Override
public void cleanDanglingDeleteFiles() {
TableConfiguration tableConfiguration = context.getTableConfiguration();
+ MaintainerMetrics metrics = context.getMetrics();
+
if (!tableConfiguration.isDeleteDanglingDeleteFilesEnabled()) {
return;
}
- Snapshot currentSnapshot = table.currentSnapshot();
- if (currentSnapshot == null) {
- return;
- }
- Optional<String> totalDeleteFiles =
-
Optional.ofNullable(currentSnapshot.summary().get(SnapshotSummary.TOTAL_DELETE_FILES_PROP));
- if (totalDeleteFiles.isPresent() && Long.parseLong(totalDeleteFiles.get())
> 0) {
- // clear dangling delete files
- doCleanDanglingDeleteFiles();
- } else {
- LOG.debug(
- "There are no delete files here, so there is no need to clean
dangling delete file for table {}",
- table.name());
- }
+ MaintainerOperationExecutor executor = new
MaintainerOperationExecutor(metrics);
+
+ executor.execute(
+ MaintainerOperationType.DANGLING_DELETE_FILES_CLEANING,
+ () -> {
+ Snapshot currentSnapshot = table.currentSnapshot();
+ if (currentSnapshot == null) {
+ return;
+ }
+ Optional<String> totalDeleteFiles =
+ Optional.ofNullable(
+
currentSnapshot.summary().get(SnapshotSummary.TOTAL_DELETE_FILES_PROP));
+ if (totalDeleteFiles.isPresent() &&
Long.parseLong(totalDeleteFiles.get()) > 0) {
+ // clear dangling delete files
+ LOG.info("Starting cleaning dangling delete files for table {}",
table.name());
+ int danglingDeleteFilesCnt =
clearInternalTableDanglingDeleteFiles();
+ runWithCondition(
+ danglingDeleteFilesCnt > 0,
+ () -> {
+ LOG.info(
+ "Deleted {} dangling delete files for table {}",
+ danglingDeleteFilesCnt,
+ table.name());
+
metrics.recordDanglingDeleteFilesCleaned(danglingDeleteFilesCnt);
+ });
+ } else {
+ LOG.debug(
+ "There are no delete files here, so there is no need to clean
dangling delete file for table {}",
+ table.name());
+ }
+ });
}
@Override
public void expireSnapshots() {
if (!expireSnapshotEnabled()) {
return;
}
- expireSnapshots(
- mustOlderThan(System.currentTimeMillis()),
- context.getTableConfiguration().getSnapshotMinCount());
- }
-
- public boolean expireSnapshotEnabled() {
- TableConfiguration tableConfiguration = context.getTableConfiguration();
- return tableConfiguration.isExpireSnapshotEnabled();
- }
+ MaintainerMetrics metrics = context.getMetrics();
+ long mustOlderThan = mustOlderThan(System.currentTimeMillis());
+ int minCount = context.getTableConfiguration().getSnapshotMinCount();
- @VisibleForTesting
- public void expireSnapshots(long mustOlderThan, int minCount) {
- expireSnapshots(mustOlderThan, minCount,
expireSnapshotNeedToExcludeFiles());
+ MaintainerOperationExecutor executor = new
MaintainerOperationExecutor(metrics);
+ executor.execute(
+ MaintainerOperationType.SNAPSHOT_EXPIRATION,
+ () -> expireSnapshotsInternal(mustOlderThan, minCount, metrics));
}
- private void expireSnapshots(long olderThan, int minCount, Set<String>
exclude) {
+ private void expireSnapshotsInternal(
+ long mustOlderThan, int minCount, MaintainerMetrics metrics) {
+ long startTime = System.currentTimeMillis();
+ Set<String> exclude = expireSnapshotNeedToExcludeFiles();
Review Comment:
Duration is recorded twice for the same operation — once by
`MaintainerOperationExecutor` (`table_maintainer_operation_duration_millis`)
and once inside this method (`table_snapshots_expiration_duration_millis`). The
two values will differ slightly. Consider using a single source of truth for
duration.
Same pattern applies to `doExpireData` and `autoCreateTags`.
--
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]
