Skip to content

[AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report #4045

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
czy006 wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report #4045

czy006 wants to merge 2 commits into from

Conversation

@czy006
Copy link
Contributor

@czy006 czy006 commented Jan 16, 2026 •
edited
Loading

[Improvement]: MaintainerMetrics Unified interface and Report (#4034)

Why are the changes needed?

Close #4034 .

Brief change log

  • MaintainerMetrics interface is abstracted to support a multi-lake architecture
  • Maintainer Metrics add other maintainer metrics interface: such as expireSnapshots/expireData/cleanDanglingDeleteFiles/autoCreateTags
  • MaintainerMetrics OrphanDataFiles method Change to a more general implementation

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)

@codecov-commenter
Copy link

codecov-commenter commented Jan 16, 2026

Codecov Report

❌ Patch coverage is 26.76768% with 145 lines in your changes missing coverage. Please review.
✅ Project coverage is 22.53%. Comparing base (cbdc517) to head (3eb329e).
⚠️ Report is 34 commits behind head on master.

Files with missing lines Patch % Lines
...ats/iceberg/maintainer/IcebergTableMaintainer.java 11.53% 136 Missing and 2 partials ⚠️
...org/apache/amoro/maintainer/MaintainerMetrics.java 25.00% 6 Missing ⚠️
...ache/amoro/maintainer/MaintainerOperationType.java 90.90% 1 Missing ⚠️
Additional details and impacted files 
@@             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 Coverage Δ
trino 22.53% <26.76%> (+0.40%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@czy006 czy006 changed the title Draft: [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report [AMORO-4034][Improvement]: MaintainerMetrics Unified interface and Report Jan 29, 2026
@czy006 czy006 requested a review from baiyangtx January 29, 2026 02:06
@czy006 czy006 force-pushed the issues/amoro-4034 branch from f9cfd81 to a4468f3 Compare January 29, 2026 02:41
@czy006 czy006 force-pushed the issues/amoro-4034 branch from a4468f3 to 0cfa9a4 Compare January 29, 2026 08:56
majin1102
majin1102 reviewed Jan 30, 2026
View reviewed changes
Copy link
Contributor

@majin1102 majin1102 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. Left some comments

amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java
*
* <p>Note: All metrics include the table_format tag to distinguish Iceberg and Paimon tables
*/
public abstract class AbstractTableMaintainerMetrics extends AbstractTableMetrics
Copy link
Contributor

@majin1102 majin1102 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering this abstraction is really necessary if we put that format field into AbstractTableMetrics. Could you elaborate more on this?

Copy link
Contributor Author

@czy006 czy006 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

amoro-ams/src/main/java/org/apache/amoro/server/table/AbstractTableMaintainerMetrics.java
defineCounter("table_orphan_data_files_cleaned_count")
.withDescription("Count of orphan data files cleaned")
.withTags("catalog", "database", "table", "table_format")
.build();
Copy link
Contributor

@majin1102 majin1102 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a break change from table_orphan_content_file_cleaning_count. This might lead to backforward compatibility issue

amoro-common/src/main/java/org/apache/amoro/maintainer/MaintainerOperationTemplate.java Outdated
@FunctionalInterface
public interface MaintainerOperation {
/** Executes the operation. */
void execute() throws Throwable;
Copy link
Contributor

@majin1102 majin1102 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

...ceberg/src/main/java/org/apache/amoro/formats/iceberg/maintainer/IcebergTableMaintainer.java Outdated
if (totalDeleteFiles.isPresent() && Long.parseLong(totalDeleteFiles.get()) > 0) {
// clear dangling delete files
doCleanDanglingDeleteFiles();
LOG.info("Starting cleaning dangling delete files for table {}", table.name());
Copy link
Contributor

@majin1102 majin1102 Jan 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So why we don't use the new interfaces and wireness?

@czy006 czy006 force-pushed the issues/amoro-4034 branch from 1a85b2d to 2e57523 Compare January 30, 2026 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@majin1102 majin1102 majin1102 left review comments

@baiyangtx baiyangtx Awaiting requested review from baiyangtx

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

module:ams-server Ams server module module:common

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Improvement]: MaintainerMetrics Unified interface and Report

3 participants

@czy006 @codecov-commenter @majin1102