[AMORO-3440] Implement the refresh and exploration of OptimizingGroup… #3457

mansonliwh · 2025-03-05T10:22:41Z

Why are the changes needed?

Close #3440 .

Brief change log

The OptimizingService and OptimizerManager have been decoupled: the OptimizerManager handles metadata persistence and modifications, while the OptimizingService includes a watcher to synchronize the in-memory OptimizingQueue map with persisted data.
This allows the master node to run the OptimizingService while other nodes modify metadata via a web interface, ensuring the master stays updated.

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 · 2025-03-06T02:02:33Z

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 21.74%. Comparing base (63aab48) to head (809bcfe).
Report is 18 commits behind head on master.

Files with missing lines	Patch %	Lines
.../java/org/apache/amoro/resource/ResourceGroup.java	0.00%	9 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3457      +/-   ##
============================================
+ Coverage     21.59%   21.74%   +0.15%     
- Complexity     2353     2386      +33     
============================================
  Files           431      431              
  Lines         40347    40476     +129     
  Branches       5711     5741      +30     
============================================
+ Hits           8712     8801      +89     
- Misses        30903    30929      +26     
- Partials        732      746      +14

Flag	Coverage Δ
trino	`21.74% <0.00%> (+0.15%)`	⬆️

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.

zhoujinsong

Thanks for the work!
I left some comments, PTAL.

amoro-ams/src/main/java/org/apache/amoro/server/resource/DefaultOptimizerManager.java

amoro-ams/src/main/java/org/apache/amoro/server/DefaultOptimizingService.java

amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java

amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java

czy006 · 2025-03-18T02:56:44Z

amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java

+          .durationType()
+          .defaultValue(Duration.ofSeconds(30))
+          .withDescription("Optimizer group refresh interval.");
+


Do we need to add this configuration to the Helm chart and configuration document (in the next PR)?

klion26 · 2025-03-18T06:13:13Z

amoro-ams/src/main/java/org/apache/amoro/server/DefaultOptimizingService.java

+        Set<String> groupNames =
+            resourceGroups.stream().map(ResourceGroup::getName).collect(Collectors.toSet());
+        Sets.difference(optimizingQueueByGroup.keySet(), groupNames)
+            .forEach(DefaultOptimizingService.this::deleteResourceGroup);


Currently, this::deleteResourceGroup will call OptimizingQueue#dispose(), it only unregister the metric, do we need to cleanup other states in OptimizingQueue there?

What else do you think should added to the disposal process?
The code looks safe according to the existing codes in the master branch.

But I am not sure if it is possible some process is open in the queue will be deleted.

Do we need to handle the tableQueue and retryTaskQueue here?
the lifetime of tableQueue and retryTaskQueue is the same as OptimizingQueue, if the queue has been disposed, maybe we should handle the resource it contains(such as tableQueue and retryTaskQueue)

The group can be dropped only if there is no table in it, so the tableQueue and retryTaskQueue should be empty.

But I am okay with adding more checks to the dispose method, we may improve it in another PR.

klion26 · 2025-03-18T06:15:32Z

amoro-ams/src/main/java/org/apache/amoro/server/resource/DefaultOptimizerManager.java

-
-  public DefaultOptimizerManager(Configurations serverConfiguration) {
+  private final CatalogManager catalogManager;
+  private final MaintainedTableManager tableManager;


it seems the tableManager does not need here

Yes, it can be dropped.

zhoujinsong

This PR looks good to me in general except some minor suggestions.

zhoujinsong · 2025-03-19T03:49:13Z

amoro-ams/src/main/java/org/apache/amoro/server/DefaultOptimizingService.java

+        Set<String> groupNames =
+            resourceGroups.stream().map(ResourceGroup::getName).collect(Collectors.toSet());
+        Sets.difference(optimizingQueueByGroup.keySet(), groupNames)
+            .forEach(DefaultOptimizingService.this::deleteResourceGroup);


What else do you think should added to the disposal process?
The code looks safe according to the existing codes in the master branch.

But I am not sure if it is possible some process is open in the queue will be deleted.

zhoujinsong · 2025-03-19T03:49:47Z

amoro-ams/src/main/java/org/apache/amoro/server/resource/DefaultOptimizerManager.java

-
-  public DefaultOptimizerManager(Configurations serverConfiguration) {
+  private final CatalogManager catalogManager;
+  private final MaintainedTableManager tableManager;


Yes, it can be dropped.

…Config

zhoujinsong

LGTM.

github-actions bot added module:ams-server Ams server module module:common labels Mar 5, 2025

mansonliwh force-pushed the optimizing-config branch 3 times, most recently from 54ff948 to 9c3fd97 Compare March 6, 2025 06:50

mansonliwh changed the title ~~draft:[AMORO-3440] Implement the refresh and exploration of OptimizingGroup…~~ [AMORO-3440] Implement the refresh and exploration of OptimizingGroup… Mar 6, 2025

czy006 requested review from baiyangtx and zhoujinsong March 10, 2025 06:39

zhoujinsong reviewed Mar 12, 2025

View reviewed changes

zhoujinsong added this to the Release 0.8.0 milestone Mar 12, 2025

github-actions bot added type:docs Improvements or additions to documentation type:build module:ams-dashboard Ams dashboard module and removed type:docs Improvements or additions to documentation type:build module:ams-dashboard Ams dashboard module labels Mar 13, 2025

baiyangtx reviewed Mar 13, 2025

View reviewed changes

amoro-ams/src/main/java/org/apache/amoro/server/persistence/mapper/TableMetaMapper.java Outdated Show resolved Hide resolved

amoro-ams/src/main/java/org/apache/amoro/server/AmoroManagementConf.java Outdated Show resolved Hide resolved

mansonliwh force-pushed the optimizing-config branch 3 times, most recently from e63e960 to 01931b8 Compare March 13, 2025 07:07

github-actions bot added the type:build label Mar 13, 2025

mansonliwh requested review from baiyangtx and zhoujinsong March 17, 2025 02:35

czy006 reviewed Mar 18, 2025

View reviewed changes

klion26 reviewed Mar 18, 2025

View reviewed changes

zhoujinsong reviewed Mar 19, 2025

View reviewed changes

mansonliwh force-pushed the optimizing-config branch from 06d87d5 to 548eadc Compare March 20, 2025 03:13

[AMORO-3440] Implement the refresh and exploration of OptimizingGroup…

5ac2bfe

…Config

mansonliwh force-pushed the optimizing-config branch from 548eadc to 5ac2bfe Compare March 20, 2025 03:19

zhoujinsong approved these changes Mar 21, 2025

View reviewed changes

zhoujinsong merged commit bce2d64 into apache:master Mar 21, 2025
4 checks passed

[AMORO-3440] Implement the refresh and exploration of OptimizingGroup… #3457

[AMORO-3440] Implement the refresh and exploration of OptimizingGroup… #3457

Uh oh!

Conversation

mansonliwh commented Mar 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

Brief change log

How was this patch tested?

Documentation

Uh oh!

codecov-commenter commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mansonliwh commented Mar 5, 2025 •

edited

Loading

codecov-commenter commented Mar 6, 2025 •

edited

Loading