Skip to content
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.

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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mansonliwh
Copy link
Contributor

@mansonliwh mansonliwh commented Mar 5, 2025

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
Copy link

codecov-commenter commented Mar 6, 2025

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.

@mansonliwh mansonliwh force-pushed the optimizing-config branch 3 times, most recently from 54ff948 to 9c3fd97 Compare March 6, 2025 06:50
@mansonliwh 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 czy006 requested review from baiyangtx and zhoujinsong March 10, 2025 06:39
Copy link
Contributor

@zhoujinsong zhoujinsong 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 the work!
I left some comments, PTAL.

@zhoujinsong zhoujinsong added this to the Release 0.8.0 milestone Mar 12, 2025
@github-actions 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
@@ -380,6 +380,12 @@ public class AmoroManagementConf {
.defaultValue(Duration.ofSeconds(3))
.withDescription("Optimizer polling task timeout.");

public static final ConfigOption<Duration> OPTIMIZER_QUEUE_REFRESH_INTERVAL =
ConfigOptions.key("optimizer.queue-refresh-interval")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ConfigOptions.key("optimizer.queue-refresh-interval")
ConfigOptions.key("optimizing-service.refresh-group-interval")

Copy link
Contributor

Choose a reason for hiding this comment

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

WDYT? @zhoujinsong

Copy link
Contributor

Choose a reason for hiding this comment

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

The name self-optimizing.refresh-group-interval seems appropriate, especially since we already have some configurations prefixed with self-optimizing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Besides, can you help to add this new configuration example to the configuration temp file.

@mansonliwh mansonliwh force-pushed the optimizing-config branch 2 times, most recently from 5a28c69 to e63e960 Compare March 13, 2025 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask]: Implement the refresh and exploration of OptimizingGroupConfig
4 participants