You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@amoro.apache.org by "czy006 (via GitHub)" <gi...@apache.org> on 2026/01/16 03:03:02 UTC
[PR] Draft: [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
czy006 opened a new pull request, #4045:
URL: https://github.com/apache/amoro/pull/4045
[Improvement]: MaintainerMetrics Unified interface and Report (#4034)
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://amoro.apache.org/how-to-contribute/
2. If the PR is related to an issue in https://github.com/apache/amoro/issues, add '[AMORO-XXXX]' in your PR title, e.g., '[AMORO-XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][AMORO-XXXX] Your PR title ...'.
-->
## Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about its use case.
2. If you fix a bug, you can clarify why it is a bug.
3. Use Fix/Resolve/Close #{ISSUE_NUMBER} to link this PR to its related issue
-->
Close #4034 .
## Brief change log
<!--
Clearly describe the changes made in modules, classes, methods, etc.
-->
-
## How was this patch tested?
- [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible
- [ ] Add screenshots for manual tests if appropriate
- [ ] Run test locally before making a pull request
## Documentation
- Does this pull request introduce a new feature? (yes / no)
- If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r2746379973
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.metrics.MetricDefine;
+
+/**
+ * Abstract base class for table maintenance operation metrics
+ *
+ * <p>Responsibilities:
+ *
+ * <ul>
+ * <li>Define all MetricDefine constants
+ * <li>Provide template methods for metrics registration
+ * <li>Handle tags (catalog, database, table, table_format, operation_type)
+ * </ul>
+ *
+ * <p>Note: All metrics include the table_format tag to distinguish Iceberg and Paimon tables
+ */
+public abstract class AbstractTableMaintainerMetrics extends AbstractTableMetrics
Review Comment:
I'm worried that on different lake formats, there might be some unique indicators of their own, so it's abstract. Although we have added the TableFormat identifier inside, this is only for monitoring and distinction. In terms of code, we cannot isolate and distinguish 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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4503724809
This pull request has been closed due to lack of activity. This is not a judgement on the merit of the PR in any way. It is just a way of keeping the PR queue manageable. If you think that is incorrect, or the pull request requires review, you can revive the PR at any time.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4017703461
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4219492320
@j1wonpark Can you help me review code ? This is an improvement to the monitoring abstraction of a multi-lake format.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4222199815
`autoCreateTags()` logic is now inlined into `IcebergTableMaintainer`, but `AutoCreateIcebergTagAction` class and its test `TestAutoCreateIcebergTagAction` are not deleted in this PR. They become dead code after this change. Should be removed.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #4045: [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report
URL: https://github.com/apache/amoro/pull/4045
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "czy006 (via GitHub)" <gi...@apache.org>.
czy006 commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4043737752
cc @baiyangtx @xxubai
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4446250293
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@amoro.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r3063048752
##########
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();
+ Set<Long> beforeSnapshotIds = snapshotIds(table.snapshots());
LOG.debug(
"Starting snapshots expiration for table {}, expiring snapshots older than {} and retain last {} snapshots, excluding {}",
table.name(),
- olderThan,
+ mustOlderThan,
minCount,
exclude);
RollingFileCleaner expiredFileCleaner = new RollingFileCleaner(fileIO(), exclude);
table
.expireSnapshots()
.retainLast(Math.max(minCount, 1))
- .expireOlderThan(olderThan)
+ .expireOlderThan(mustOlderThan)
.deleteWith(expiredFileCleaner::addFile)
.cleanExpiredFiles(
true) /* enable clean only for collecting the expired files, will delete them later */
.commit();
- int collectedFiles = expiredFileCleaner.fileCount();
expiredFileCleaner.clear();
- if (collectedFiles > 0) {
+ int dataFilesDeleted = expiredFileCleaner.cleanedFileCount();
+ table.refresh();
+ Set<Long> afterSnapshotIds = snapshotIds(table.snapshots());
+ beforeSnapshotIds.removeAll(afterSnapshotIds);
+ int expiredSnapshotCount = beforeSnapshotIds.size();
+
+ long duration = System.currentTimeMillis() - startTime;
+
+ if (expiredSnapshotCount > 0) {
LOG.info(
- "Expired {}/{} files for table {} order than {}",
- collectedFiles,
- expiredFileCleaner.cleanedFileCount(),
+ "Expired {} snapshots and deleted {} files for table {} older than {}",
+ expiredSnapshotCount,
+ dataFilesDeleted,
table.name(),
- DateTimeUtil.formatTimestampMillis(olderThan));
+ DateTimeUtil.formatTimestampMillis(mustOlderThan));
+ metrics.recordSnapshotsExpired(expiredSnapshotCount, dataFilesDeleted, duration);
} else {
LOG.debug(
- "No expired files found for table {} order than {}",
+ "No expired files found for table {} older than {}",
table.name(),
- DateTimeUtil.formatTimestampMillis(olderThan));
+ DateTimeUtil.formatTimestampMillis(mustOlderThan));
}
}
+ public boolean expireSnapshotEnabled() {
+ TableConfiguration tableConfiguration = context.getTableConfiguration();
+ return tableConfiguration.isExpireSnapshotEnabled();
+ }
+
+ @VisibleForTesting
+ public void expireSnapshots(long mustOlderThan, int minCount) {
+ MaintainerMetrics metrics = context.getMetrics();
+ expireSnapshotsInternal(mustOlderThan, minCount, metrics);
+ }
+
+ private void expireSnapshots(long olderThan, int minCount, Set<String> exclude) {
+ // This method is kept for backward compatibility with potential subclasses
Review Comment:
This private method is no longer called after the refactor (2-param `expireSnapshots` now calls `expireSnapshotsInternal` directly). The "backward compatibility with subclasses" comment doesn't apply since it's `private`. Can be removed.
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/TableMaintainerMetrics.java:
##########
@@ -0,0 +1,531 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.maintainer.MaintainerOperationType;
+import org.apache.amoro.metrics.Counter;
+import org.apache.amoro.metrics.Gauge;
+import org.apache.amoro.metrics.Metric;
+import org.apache.amoro.metrics.MetricDefine;
+import org.apache.amoro.metrics.MetricKey;
+import org.apache.amoro.metrics.MetricRegistry;
+import org.apache.amoro.shade.guava32.com.google.common.collect.ImmutableMap;
+import org.apache.amoro.shade.guava32.com.google.common.collect.Lists;
+
+import java.util.List;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+/**
+ * Table maintenance operation metrics implementation.
+ *
+ * <p>This class handles metrics recording for table maintenance operations such as orphan file
+ * cleaning, snapshot expiration, data expiration, and tag creation.
+ *
+ * <p>Design notes:
+ *
+ * <ul>
+ * <li>Each maintenance operation corresponds to a set of metrics (Counter + Gauge)
+ * <li>Counter is used for cumulative counting (e.g., file count)
+ * <li>Gauge is used to record current state (e.g., execution duration)
+ * <li>Supports thread safety in concurrent scenarios
+ * <li>Includes table_format tag to distinguish Iceberg and Paimon tables
+ * </ul>
+ */
+public class TableMaintainerMetrics implements MaintainerMetrics {
+
+ // ========== Orphan Files Related MetricDefine ==========
+
+ /**
+ * Count of orphan content files cleaned.
+ *
+ * <p>Note: This metric name is retained for backward compatibility. The "content" terminology
+ * refers to data files (as opposed to metadata files).
+ */
+ public static final MetricDefine TABLE_ORPHAN_CONTENT_FILE_CLEANING_COUNT =
+ defineCounter("table_orphan_content_file_cleaning_count")
+ .withDescription("Count of orphan content files cleaned")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /**
+ * Expected count of orphan content files to clean.
+ *
+ * <p>Note: This metric name is retained for backward compatibility. The "content" terminology
+ * refers to data files (as opposed to metadata files).
+ */
+ public static final MetricDefine TABLE_EXPECTED_ORPHAN_CONTENT_FILE_CLEANING_COUNT =
+ defineCounter("table_expected_orphan_content_file_cleaning_count")
+ .withDescription("Expected count of orphan content files to clean")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Count of orphan metadata files cleaned */
+ public static final MetricDefine TABLE_ORPHAN_METADATA_FILES_CLEANED_COUNT =
+ // Keep historical metric name for backward compatibility.
+ defineCounter("table_orphan_metadata_file_cleaning_count")
+ .withDescription("Count of orphan metadata files cleaned")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Expected count of orphan metadata files to clean */
+ public static final MetricDefine TABLE_ORPHAN_METADATA_FILES_CLEANED_EXPECTED_COUNT =
+ // Keep historical metric name for backward compatibility.
+ defineCounter("table_expected_orphan_metadata_file_cleaning_count")
+ .withDescription("Expected count of orphan metadata files to clean")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of orphan files cleaning operation (milliseconds) */
+ public static final MetricDefine TABLE_ORPHAN_FILES_CLEANING_DURATION =
+ defineGauge("table_orphan_files_cleaning_duration_millis")
+ .withDescription("Duration of orphan files cleaning operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== Dangling Delete Files Related MetricDefine (Iceberg) ==========
+
+ /** Count of dangling delete files cleaned */
+ public static final MetricDefine TABLE_DANGLING_DELETE_FILES_CLEANED_COUNT =
+ defineCounter("table_dangling_delete_files_cleaned_count")
+ .withDescription("Count of dangling delete files cleaned")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of dangling delete files cleaning operation (milliseconds) */
+ public static final MetricDefine TABLE_DANGLING_DELETE_FILES_CLEANING_DURATION =
+ defineGauge("table_dangling_delete_files_cleaning_duration_millis")
+ .withDescription("Duration of dangling delete files cleaning operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== Snapshot Expiration Related MetricDefine ==========
+
+ /** Count of snapshots expired */
+ public static final MetricDefine TABLE_SNAPSHOTS_EXPIRED_COUNT =
+ defineCounter("table_snapshots_expired_count")
+ .withDescription("Count of snapshots expired")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Count of data files deleted during snapshot expiration */
+ public static final MetricDefine TABLE_SNAPSHOTS_EXPIRED_DATA_FILES_DELETED =
+ defineCounter("table_snapshots_expired_data_files_deleted")
+ .withDescription("Count of data files deleted during snapshot expiration")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of snapshot expiration operation (milliseconds) */
+ public static final MetricDefine TABLE_SNAPSHOTS_EXPIRATION_DURATION =
+ defineGauge("table_snapshots_expiration_duration_millis")
+ .withDescription("Duration of snapshot expiration operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== Data Expiration Related MetricDefine (Iceberg) ==========
+
+ /** Count of data files expired */
+ public static final MetricDefine TABLE_DATA_EXPIRED_DATA_FILES_COUNT =
+ defineCounter("table_data_expired_data_files_count")
+ .withDescription("Count of data files expired")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Count of delete files expired */
+ public static final MetricDefine TABLE_DATA_EXPIRED_DELETE_FILES_COUNT =
+ defineCounter("table_data_expired_delete_files_count")
+ .withDescription("Count of delete files expired")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of data expiration operation (milliseconds) */
+ public static final MetricDefine TABLE_DATA_EXPIRATION_DURATION =
+ defineGauge("table_data_expiration_duration_millis")
+ .withDescription("Duration of data expiration operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== Tag Creation Related MetricDefine (Iceberg) ==========
+
+ /** Count of tags created */
+ public static final MetricDefine TABLE_TAGS_CREATED_COUNT =
+ defineCounter("table_tags_created_count")
+ .withDescription("Count of tags created")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of tag creation operation (milliseconds) */
+ public static final MetricDefine TABLE_TAG_CREATION_DURATION =
+ defineGauge("table_tag_creation_duration_millis")
+ .withDescription("Duration of tag creation operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== Partition Expiration Related MetricDefine (Paimon) ==========
+
+ /** Count of partitions expired */
+ public static final MetricDefine TABLE_PARTITIONS_EXPIRED_COUNT =
+ defineCounter("table_partitions_expired_count")
+ .withDescription("Count of partitions expired")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Count of files expired during partition expiration */
+ public static final MetricDefine TABLE_PARTITIONS_EXPIRED_FILES_COUNT =
+ defineCounter("table_partitions_expired_files_count")
+ .withDescription("Count of files expired during partition expiration")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ /** Duration of partition expiration operation (milliseconds) */
+ public static final MetricDefine TABLE_PARTITION_EXPIRATION_DURATION =
+ defineGauge("table_partition_expiration_duration_millis")
+ .withDescription("Duration of partition expiration operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
+
+ // ========== General Operation Status Related MetricDefine ==========
+
+ /** Count of successful maintainer operations */
+ public static final MetricDefine TABLE_MAINTAINER_OPERATION_SUCCESS_COUNT =
+ defineCounter("table_maintainer_operation_success_count")
+ .withDescription("Count of successful maintainer operations")
+ .withTags("catalog", "database", "table", "table_format", "operation_type")
+ .build();
+
+ /** Count of failed maintainer operations */
+ public static final MetricDefine TABLE_MAINTAINER_OPERATION_FAILURE_COUNT =
+ defineCounter("table_maintainer_operation_failure_count")
+ .withDescription("Count of failed maintainer operations")
+ .withTags("catalog", "database", "table", "table_format", "operation_type")
+ .build();
+
+ /** Duration of maintainer operation (milliseconds) */
+ public static final MetricDefine TABLE_MAINTAINER_OPERATION_DURATION =
+ defineGauge("table_maintainer_operation_duration_millis")
+ .withDescription("Duration of maintainer operation in milliseconds")
+ .withTags("catalog", "database", "table", "table_format", "operation_type")
+ .build();
+
+ // ========== Instance Fields ==========
+
+ private final ServerTableIdentifier identifier;
+ private final String tableFormat;
+ private final List<MetricKey> registeredMetricKeys = Lists.newArrayList();
+ private MetricRegistry globalRegistry;
+
+ // ========== Orphan Files Metrics ==========
+ private final Counter orphanContentFileCleaningCount = new Counter();
+ private final Counter expectedOrphanContentFileCleaningCount = new Counter();
+ private final Counter orphanMetadataFilesCount = new Counter();
+ private final Counter orphanMetadataFilesExpectedCount = new Counter();
+ private final LastOperationDurationGauge orphanFilesCleaningDuration =
+ new LastOperationDurationGauge();
+
+ // ========== Dangling Delete Files Metrics ==========
+ private final Counter danglingDeleteFilesCount = new Counter();
+ private final LastOperationDurationGauge danglingDeleteFilesCleaningDuration =
+ new LastOperationDurationGauge();
+
+ // ========== Snapshot Expiration Metrics ==========
+ private final Counter snapshotsExpiredCount = new Counter();
+ private final Counter snapshotsExpiredDataFilesDeleted = new Counter();
+ private final LastOperationDurationGauge snapshotsExpirationDuration =
+ new LastOperationDurationGauge();
+
+ // ========== Data Expiration Metrics ==========
+ private final Counter dataExpiredDataFilesCount = new Counter();
+ private final Counter dataExpiredDeleteFilesCount = new Counter();
+ private final LastOperationDurationGauge dataExpirationDuration =
+ new LastOperationDurationGauge();
+
+ // ========== Tag Creation Metrics ==========
+ private final Counter tagsCreatedCount = new Counter();
+ private final LastOperationDurationGauge tagCreationDuration = new LastOperationDurationGauge();
+
+ // ========== Partition Expiration Metrics ==========
+ private final Counter partitionsExpiredCount = new Counter();
+ private final Counter partitionsExpiredFilesCount = new Counter();
+ private final LastOperationDurationGauge partitionExpirationDuration =
+ new LastOperationDurationGauge();
+
+ // ========== Operation Status Metrics ==========
+ private final ConcurrentHashMap<MaintainerOperationType, Counter> successCounters =
+ new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<MaintainerOperationType, Counter> failureCounters =
+ new ConcurrentHashMap<>();
+ private final ConcurrentHashMap<MaintainerOperationType, OperationDurationGauge> durationGauges =
+ new ConcurrentHashMap<>();
+
+ /**
+ * Constructor
+ *
+ * @param identifier Table identifier (contains format information via getFormat())
+ */
+ public TableMaintainerMetrics(ServerTableIdentifier identifier) {
+ this.identifier = identifier;
+ this.tableFormat = identifier.getFormat().name().toLowerCase();
+ // Initialize operation type counters and gauges
+ for (MaintainerOperationType type : MaintainerOperationType.values()) {
+ successCounters.put(type, new Counter());
+ failureCounters.put(type, new Counter());
+ durationGauges.put(type, new OperationDurationGauge());
+ }
+ }
+
+ /**
+ * Get the table identifier.
+ *
+ * @return ServerTableIdentifier
+ */
+ public ServerTableIdentifier getIdentifier() {
+ return identifier;
+ }
+
+ public void register(MetricRegistry registry) {
+ if (globalRegistry != null) {
+ return;
+ }
+ registerMetrics(registry);
+ globalRegistry = registry;
+ }
+
+ public void unregister() {
+ if (globalRegistry != null) {
+ registeredMetricKeys.forEach(globalRegistry::unregister);
+ registeredMetricKeys.clear();
+ globalRegistry = null;
+ }
+ }
+
+ private void registerMetrics(MetricRegistry registry) {
+ // Build base tags (including table_format)
+ Map<String, String> baseTags =
+ ImmutableMap.of(
+ "catalog",
+ identifier.getCatalog(),
+ "database",
+ identifier.getDatabase(),
+ "table",
+ identifier.getTableName(),
+ "table_format",
+ tableFormat);
+
+ // Orphan files
+ registerMetricWithTags(
+ registry,
+ TABLE_ORPHAN_CONTENT_FILE_CLEANING_COUNT,
+ orphanContentFileCleaningCount,
+ baseTags);
+ registerMetricWithTags(
+ registry,
+ TABLE_EXPECTED_ORPHAN_CONTENT_FILE_CLEANING_COUNT,
+ expectedOrphanContentFileCleaningCount,
+ baseTags);
+ registerMetricWithTags(
+ registry, TABLE_ORPHAN_METADATA_FILES_CLEANED_COUNT, orphanMetadataFilesCount, baseTags);
+ registerMetricWithTags(
+ registry,
+ TABLE_ORPHAN_METADATA_FILES_CLEANED_EXPECTED_COUNT,
+ orphanMetadataFilesExpectedCount,
+ baseTags);
+ registerMetricWithTags(
+ registry, TABLE_ORPHAN_FILES_CLEANING_DURATION, orphanFilesCleaningDuration, baseTags);
+
+ // Dangling delete files
+ registerMetricWithTags(
+ registry, TABLE_DANGLING_DELETE_FILES_CLEANED_COUNT, danglingDeleteFilesCount, baseTags);
+ registerMetricWithTags(
+ registry,
+ TABLE_DANGLING_DELETE_FILES_CLEANING_DURATION,
+ danglingDeleteFilesCleaningDuration,
+ baseTags);
+
+ // Snapshot expiration
+ registerMetricWithTags(
+ registry, TABLE_SNAPSHOTS_EXPIRED_COUNT, snapshotsExpiredCount, baseTags);
+ registerMetricWithTags(
+ registry,
+ TABLE_SNAPSHOTS_EXPIRED_DATA_FILES_DELETED,
+ snapshotsExpiredDataFilesDeleted,
+ baseTags);
+ registerMetricWithTags(
+ registry, TABLE_SNAPSHOTS_EXPIRATION_DURATION, snapshotsExpirationDuration, baseTags);
+
+ // Data expiration
+ registerMetricWithTags(
+ registry, TABLE_DATA_EXPIRED_DATA_FILES_COUNT, dataExpiredDataFilesCount, baseTags);
+ registerMetricWithTags(
+ registry, TABLE_DATA_EXPIRED_DELETE_FILES_COUNT, dataExpiredDeleteFilesCount, baseTags);
+ registerMetricWithTags(
+ registry, TABLE_DATA_EXPIRATION_DURATION, dataExpirationDuration, baseTags);
+
+ // Tag creation
+ registerMetricWithTags(registry, TABLE_TAGS_CREATED_COUNT, tagsCreatedCount, baseTags);
+ registerMetricWithTags(registry, TABLE_TAG_CREATION_DURATION, tagCreationDuration, baseTags);
+
+ // Partition expiration
+ registerMetricWithTags(
+ registry, TABLE_PARTITIONS_EXPIRED_COUNT, partitionsExpiredCount, baseTags);
+ registerMetricWithTags(
+ registry, TABLE_PARTITIONS_EXPIRED_FILES_COUNT, partitionsExpiredFilesCount, baseTags);
+ registerMetricWithTags(
+ registry, TABLE_PARTITION_EXPIRATION_DURATION, partitionExpirationDuration, baseTags);
+
+ // Operation status (needs to include operation_type tag)
+ for (MaintainerOperationType type : MaintainerOperationType.values()) {
+ Map<String, String> operationTags =
+ ImmutableMap.<String, String>builder()
+ .putAll(baseTags)
+ .put("operation_type", type.getMetricName())
+ .build();
+ registerMetricWithTags(
+ registry,
+ TABLE_MAINTAINER_OPERATION_SUCCESS_COUNT,
+ successCounters.get(type),
+ operationTags);
+ registerMetricWithTags(
+ registry,
+ TABLE_MAINTAINER_OPERATION_FAILURE_COUNT,
+ failureCounters.get(type),
+ operationTags);
+ registerMetricWithTags(
+ registry, TABLE_MAINTAINER_OPERATION_DURATION, durationGauges.get(type), operationTags);
+ }
+ }
+
+ /**
+ * Register metric with specified tags
+ *
+ * @param registry MetricRegistry
+ * @param define MetricDefine
+ * @param metric Metric instance
+ * @param tags Tags
+ */
+ private void registerMetricWithTags(
+ MetricRegistry registry, MetricDefine define, Metric metric, Map<String, String> tags) {
+ MetricKey key = registry.register(define, tags, metric);
+ registeredMetricKeys.add(key);
+ }
+
+ // ========== MaintainerMetrics Interface Implementation ==========
+
+ @Override
+ public void recordOrphanDataFilesCleaned(int expected, int cleaned) {
+ expectedOrphanContentFileCleaningCount.inc(expected);
+ orphanContentFileCleaningCount.inc(cleaned);
+ }
+
+ @Override
+ public void recordOrphanMetadataFilesCleaned(int expected, int cleaned) {
+ orphanMetadataFilesExpectedCount.inc(expected);
+ orphanMetadataFilesCount.inc(cleaned);
+ }
+
+ @Override
+ public void recordDanglingDeleteFilesCleaned(int cleaned) {
+ danglingDeleteFilesCount.inc(cleaned);
+ }
+
+ @Override
+ public void recordSnapshotsExpired(int snapshotCount, int dataFilesDeleted, long durationMillis) {
+ snapshotsExpiredCount.inc(snapshotCount);
+ snapshotsExpiredDataFilesDeleted.inc(dataFilesDeleted);
+ snapshotsExpirationDuration.setValue(durationMillis);
+ }
+
+ @Override
+ public void recordDataExpired(int dataFilesExpired, int deleteFilesExpired, long durationMillis) {
+ dataExpiredDataFilesCount.inc(dataFilesExpired);
+ dataExpiredDeleteFilesCount.inc(deleteFilesExpired);
+ dataExpirationDuration.setValue(durationMillis);
+ }
+
+ @Override
+ public void recordTagsCreated(int tagsCreated, long durationMillis) {
+ tagsCreatedCount.inc(tagsCreated);
+ tagCreationDuration.setValue(durationMillis);
+ }
+
+ @Override
+ public void recordPartitionsExpired(
+ int partitionsExpired, int filesExpired, long durationMillis) {
+ partitionsExpiredCount.inc(partitionsExpired);
+ partitionsExpiredFilesCount.inc(filesExpired);
+ partitionExpirationDuration.setValue(durationMillis);
+ }
+
+ @Override
+ public void recordOperationStart(MaintainerOperationType operationType) {
+ durationGauges.get(operationType).recordStart();
+ }
+
+ @Override
+ public void recordOperationSuccess(MaintainerOperationType operationType, long durationMillis) {
+ successCounters.get(operationType).inc();
+ durationGauges.get(operationType).setValue(durationMillis);
+ }
+
+ @Override
+ public void recordOperationFailure(
+ MaintainerOperationType operationType, long durationMillis, Throwable throwable) {
+ failureCounters.get(operationType).inc();
+ durationGauges.get(operationType).setValue(durationMillis);
+ }
+
+ // ========== Internal Helper Classes ==========
+
+ /** Gauge implementation for recording last operation duration */
+ private static class LastOperationDurationGauge implements Gauge<Long> {
+ private volatile long value = 0L;
+
+ public void setValue(long value) {
+ this.value = value;
+ }
+
+ @Override
+ public Long getValue() {
+ return value;
+ }
+ }
+
+ /** Gauge implementation for recording operation duration (supports recording start time) */
+ private static class OperationDurationGauge implements Gauge<Long> {
+ private volatile long value = 0L;
+ private volatile long startTime = 0L;
+
Review Comment:
`startTime` is written in `recordStart()` but never read — `getValue()` only returns `value`. Looks like a leftover from an earlier design. Can be removed along with `recordStart()`.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "majin1102 (via GitHub)" <gi...@apache.org>.
majin1102 commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r2745017717
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType, MaintainerOperation operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return true;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording and return result.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @param <T> the result type
+ * @return the operation result
+ * @throws Throwable if the operation throws an exception
+ */
+ public <T> T executeAndReturn(
+ MaintainerOperationType operationType, MaintainerOperationWithResult<T> operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ T result = operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return result;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /** Functional interface for maintainer operation without return value. */
+ @FunctionalInterface
+ public interface MaintainerOperation {
+ /** Executes the operation. */
+ void execute() throws Throwable;
+ }
+
+ /**
+ * Functional interface for maintainer operation with return value.
+ *
+ * @param <T> the result type
+ */
+ @FunctionalInterface
+ public interface MaintainerOperationWithResult<T> {
Review Comment:
I think we could make this more reasonable. like MaintainerCallable<T> and MaintainerRunner
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "majin1102 (via GitHub)" <gi...@apache.org>.
majin1102 commented on code in PR #4045:
URL: https://github.com/apache/amoro/pull/4045#discussion_r2744940597
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -273,26 +328,191 @@ public void expireDataFrom(DataExpirationConfig expirationConfig, Instant instan
if (instant.equals(Instant.MIN)) {
return;
}
+ MaintainerMetrics metrics = context.getMetrics();
+ long startTime = System.currentTimeMillis();
+ expireDataFromWithMetrics(expirationConfig, instant, metrics, startTime);
+ }
+ private void expireDataFromWithMetrics(
+ DataExpirationConfig expirationConfig,
+ Instant instant,
+ MaintainerMetrics metrics,
+ long startTime) {
Review Comment:
I think we might need
```
if (instant.equals(Instant.MIN)) {
return;
}
```
What do you think
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -273,26 +328,191 @@ public void expireDataFrom(DataExpirationConfig expirationConfig, Instant instan
if (instant.equals(Instant.MIN)) {
return;
}
+ MaintainerMetrics metrics = context.getMetrics();
+ long startTime = System.currentTimeMillis();
+ expireDataFromWithMetrics(expirationConfig, instant, metrics, startTime);
+ }
+ private void expireDataFromWithMetrics(
Review Comment:
I think `expireDataFrom` is just OK. It's a little werid to me to use `FromWith`
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.metrics.MetricDefine;
+
+/**
+ * Abstract base class for table maintenance operation metrics
+ *
+ * <p>Responsibilities:
+ *
+ * <ul>
+ * <li>Define all MetricDefine constants
+ * <li>Provide template methods for metrics registration
+ * <li>Handle tags (catalog, database, table, table_format, operation_type)
+ * </ul>
+ *
+ * <p>Note: All metrics include the table_format tag to distinguish Iceberg and Paimon tables
+ */
+public abstract class AbstractTableMaintainerMetrics extends AbstractTableMetrics
+ implements MaintainerMetrics {
+
+ /** Table format constant: Iceberg native table */
+ protected static final String TABLE_FORMAT_ICEBERG = "iceberg";
+
+ /** Table format constant: Paimon native table */
+ protected static final String TABLE_FORMAT_PAIMON = "paimon";
+
+ /** Table format constant: Mixed table (based on Iceberg) */
+ protected static final String TABLE_FORMAT_MIXED_ICEBERG = "mixed_iceberg";
+
+ /** Table format constant: Hive table */
+ protected static final String TABLE_FORMAT_HIVE = "hive";
+
+ // ========== Orphan Files Related MetricDefine ==========
+
+ /** Count of orphan data files cleaned */
+ public static final MetricDefine TABLE_ORPHAN_DATA_FILES_CLEANED_COUNT =
+ defineCounter("table_orphan_data_files_cleaned_count")
+ .withDescription("Count of orphan data files cleaned")
+ .withTags("catalog", "database", "table", "table_format")
+ .build();
Review Comment:
This seems a break change from `table_orphan_content_file_cleaning_count`. This might lead to backforward compatibility issue
##########
amoro-format-iceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java:
##########
@@ -165,7 +168,17 @@ public void cleanDanglingDeleteFiles() {
Optional.ofNullable(currentSnapshot.summary().get(SnapshotSummary.TOTAL_DELETE_FILES_PROP));
if (totalDeleteFiles.isPresent() && Long.parseLong(totalDeleteFiles.get()) > 0) {
// clear dangling delete files
- doCleanDanglingDeleteFiles();
+ LOG.info("Starting cleaning dangling delete files for table {}", table.name());
Review Comment:
So why we don't use the new interfaces and wireness?
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
Review Comment:
This class looks more like an executor that wraps maintainer operations with metrics, rather than a classic template-method base class. To make the intent clearer, I suggest renaming it to `MaintainerOperationExecutor` (or MaintainerMetricsExecutor, MaintainerOperationRunner), which better reflects that it mainly wires metrics around the operation execution.
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType, MaintainerOperation operation)
Review Comment:
This should be a void
##########
amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java:
##########
@@ -0,0 +1,229 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.server.table;
+
+import static org.apache.amoro.metrics.MetricDefine.defineCounter;
+import static org.apache.amoro.metrics.MetricDefine.defineGauge;
+
+import org.apache.amoro.ServerTableIdentifier;
+import org.apache.amoro.maintainer.MaintainerMetrics;
+import org.apache.amoro.metrics.MetricDefine;
+
+/**
+ * Abstract base class for table maintenance operation metrics
+ *
+ * <p>Responsibilities:
+ *
+ * <ul>
+ * <li>Define all MetricDefine constants
+ * <li>Provide template methods for metrics registration
+ * <li>Handle tags (catalog, database, table, table_format, operation_type)
+ * </ul>
+ *
+ * <p>Note: All metrics include the table_format tag to distinguish Iceberg and Paimon tables
+ */
+public abstract class AbstractTableMaintainerMetrics extends AbstractTableMetrics
Review Comment:
Wondering this abstraction is really necessary if we put that format field into `AbstractTableMetrics`. Could you elaborate more on this?
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType, MaintainerOperation operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return true;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording and return result.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @param <T> the result type
+ * @return the operation result
+ * @throws Throwable if the operation throws an exception
+ */
+ public <T> T executeAndReturn(
+ MaintainerOperationType operationType, MaintainerOperationWithResult<T> operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ T result = operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return result;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /** Functional interface for maintainer operation without return value. */
+ @FunctionalInterface
+ public interface MaintainerOperation {
+ /** Executes the operation. */
+ void execute() throws Throwable;
+ }
+
+ /**
+ * Functional interface for maintainer operation with return value.
+ *
+ * @param <T> the result type
+ */
+ @FunctionalInterface
+ public interface MaintainerOperationWithResult<T> {
Review Comment:
This interface does not make sense to me
##########
amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java:
##########
@@ -0,0 +1,151 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.amoro.maintainer;
+
+/**
+ * Template for executing maintainer operations with consistent metrics recording.
+ *
+ * <p>This template ensures that all maintainer operations record metrics in a consistent way:
+ *
+ * <ul>
+ * <li>Record operation start
+ * <li>Execute the operation
+ * <li>Record operation success/failure with duration
+ * </ul>
+ *
+ * <p>Usage example:
+ *
+ * <pre>{@code
+ * MaintainerOperationTemplate template = new MaintainerOperationTemplate(metrics);
+ * template.execute(
+ * MaintainerOperationType.ORPHAN_FILES_CLEANING,
+ * () -> {
+ * // Operation logic here
+ * cleanOrphanFiles();
+ * }
+ * );
+ * }</pre>
+ */
+public class MaintainerOperationTemplate {
+
+ private final MaintainerMetrics metrics;
+
+ /**
+ * Creates a new operation template with the given metrics collector.
+ *
+ * @param metrics the metrics collector (can be null, will use NOOP in that case)
+ */
+ public MaintainerOperationTemplate(MaintainerMetrics metrics) {
+ this.metrics = metrics != null ? metrics : MaintainerMetrics.NOOP;
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @return true if operation succeeded, false otherwise
+ * @throws Throwable if the operation throws an exception
+ */
+ public boolean execute(MaintainerOperationType operationType, MaintainerOperation operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return true;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /**
+ * Executes a maintainer operation with metrics recording and return result.
+ *
+ * <p>This method will:
+ *
+ * <ol>
+ * <li>Record the operation start via {@link MaintainerMetrics#recordOperationStart}
+ * <li>Execute the provided operation
+ * <li>On success: record operation success via {@link MaintainerMetrics#recordOperationSuccess}
+ * <li>On failure: record operation failure via {@link MaintainerMetrics#recordOperationFailure}
+ * and rethrow the exception
+ * </ol>
+ *
+ * @param operationType the type of operation being executed
+ * @param operation the operation to execute
+ * @param <T> the result type
+ * @return the operation result
+ * @throws Throwable if the operation throws an exception
+ */
+ public <T> T executeAndReturn(
+ MaintainerOperationType operationType, MaintainerOperationWithResult<T> operation)
+ throws Throwable {
+ long startTime = System.currentTimeMillis();
+ metrics.recordOperationStart(operationType);
+
+ try {
+ T result = operation.execute();
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationSuccess(operationType, duration);
+ return result;
+ } catch (Throwable t) {
+ long duration = System.currentTimeMillis() - startTime;
+ metrics.recordOperationFailure(operationType, duration, t);
+ throw t;
+ }
+ }
+
+ /** Functional interface for maintainer operation without return value. */
+ @FunctionalInterface
+ public interface MaintainerOperation {
+ /** Executes the operation. */
+ void execute() throws Throwable;
Review Comment:
I think It's a little strange to declare as `throws Throwable`.
Does any project declare like this?
And I think we could just use some interfaces JDK provides, like Runnable?
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-3757904845
## [Codecov](https://app.codecov.io/gh/apache/amoro/pull/4045?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `26.76768%` with `145 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 22.53%. Comparing base ([`cbdc517`](https://app.codecov.io/gh/apache/amoro/commit/cbdc5176b760b2ab46daa5051f29b72f1fee7175?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`3eb329e`](https://app.codecov.io/gh/apache/amoro/commit/3eb329ecfa5d2367f9041d81973f0cb781400add?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 34 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/amoro/pull/4045?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...ats/iceberg/maintainer/IcebergTableMaintainer.java](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&filepath=amoro-format-iceberg%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fformats%2Ficeberg%2Fmaintainer%2FIcebergTableMaintainer.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tZm9ybWF0LWljZWJlcmcvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Ftb3JvL2Zvcm1hdHMvaWNlYmVyZy9tYWludGFpbmVyL0ljZWJlcmdUYWJsZU1haW50YWluZXIuamF2YQ==) | 11.53% | [136 Missing and 2 partials :warning: ](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...org/apache/amoro/maintainer/MaintainerMetrics.java](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&filepath=amoro-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fmaintainer%2FMaintainerMetrics.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9hbW9yby9tYWludGFpbmVyL01haW50YWluZXJNZXRyaWNzLmphdmE=) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...ache/amoro/maintainer/MaintainerOperationType.java](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&filepath=amoro-common%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Famoro%2Fmaintainer%2FMaintainerOperationType.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-YW1vcm8tY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9hbW9yby9tYWludGFpbmVyL01haW50YWluZXJPcGVyYXRpb25UeXBlLmphdmE=) | 90.90% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/amoro/pull/4045?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4045 +/- ##
============================================
+ Coverage 22.12% 22.53% +0.40%
- Complexity 2461 2570 +109
============================================
Files 445 459 +14
Lines 40897 42239 +1342
Branches 5767 5924 +157
============================================
+ Hits 9050 9520 +470
- Misses 31089 31902 +813
- Partials 758 817 +59
```
| [Flag](https://app.codecov.io/gh/apache/amoro/pull/4045/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [trino](https://app.codecov.io/gh/apache/amoro/pull/4045/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `22.53% <26.76%> (+0.40%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/amoro/pull/4045?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [amoro]
Posted by "j1wonpark (via GitHub)" <gi...@apache.org>.
j1wonpark commented on PR #4045:
URL: https://github.com/apache/amoro/pull/4045#issuecomment-4222286794
Thanks for the contribution! I left a few inline comments on dead code and duration recording.
One design suggestion: the tag creation logic from `AutoCreateIcebergTagAction` is now inlined into `IcebergTableMaintainer`, which makes the class larger, leaves the original class as dead code, and couples tag creation logic with the maintainer. It might be cleaner to keep `AutoCreateIcebergTagAction` and extend it to support metrics, keeping the responsibilities decoupled.
--
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: commits-unsubscribe@amoro.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org