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-3329] Add MemorySize for parsing memory value and unit #3409

Merged

Conversation

Jzjsnow
Copy link
Contributor

@Jzjsnow Jzjsnow commented Jan 17, 2025

Why are the changes needed?

Currently, when we use flink optimizer groups, the memory parameters flink-conf.jobmanager.memory.process.size and flink-conf.taskmanager.memory.process.size only support plain numeric strings or strings with m/g suffix. If we configure them with mb/gb suffix, we'll get wrong parsed results. And the default unit for plain numeric strings is mb, which is inconsistent with the default unit of byte in the flink native parameter and may cause inconvenience for users familiar with flink when using this parameter.

Close #3329.

Brief change log

  • Add a new class MemorySize to the amoro-common module for parsing flink's memory parameters, consistent with flink's native parsing logic.
  • Update the values in the column properties of ams metadata tables resource_group and resource to automatically add the MB suffix to the flink jm and tm memory parameters that have been set and saved as plain numeric strings

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)

@Jzjsnow Jzjsnow force-pushed the add_MemorySize_for_memory_value_and_unit branch from df49415 to 9334783 Compare January 17, 2025 02:37
@codecov-commenter
Copy link

codecov-commenter commented Jan 17, 2025

Codecov Report

Attention: Patch coverage is 76.59574% with 33 lines in your changes missing coverage. Please review.

Project coverage is 27.72%. Comparing base (243d289) to head (9334783).
Report is 30 commits behind head on master.

Files with missing lines Patch % Lines
...c/main/java/org/apache/amoro/utils/MemorySize.java 77.53% 26 Missing and 5 partials ⚠️
.../amoro/server/manager/FlinkOptimizerContainer.java 33.33% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3409      +/-   ##
============================================
+ Coverage     21.59%   27.72%   +6.13%     
- Complexity     2309     3566    +1257     
============================================
  Files           426      596     +170     
  Lines         39719    48528    +8809     
  Branches       5624     6263     +639     
============================================
+ Hits           8577    13456    +4879     
- Misses        30414    34133    +3719     
- Partials        728      939     +211     
Flag Coverage Δ
core 27.72% <76.59%> (?)
trino ?

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.

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 a lot for the contribution!

I think this is a good start to make our configuration value more readable.
We may improve our configuration format in other place, like the AMS configuration after this PR been merged.

I left a minor suggestion, you may want have a look!

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.

LGTM.

Copy link
Member

@klion26 klion26 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 contribution, LGTM

This patch is back-compatible, but it contains some user behavior change, maybe we need to add this in the release notes, @Jzjsnow could you please create an issue to track the release notes update, thanks.

Apart the above, do we need to add some documentation about how to configure this in the documentation? if this is yes could you please add an issue to track it. thanks.

@zhoujinsong zhoujinsong merged commit 3896bbe into apache:master Jan 21, 2025
4 checks passed
zhoujinsong added a commit to zhoujinsong/amoro that referenced this pull request Jan 21, 2025
…#3409)

* [AMORO-3329] Add MemorySize for parsing memory value and unit

* fixup! [AMORO-3329] Add MemorySize for parsing memory value and unit

---------

Co-authored-by: jzjsnow <[email protected]>
Co-authored-by: ZhouJinsong <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement]: Add MemorySize for memory value and unit
4 participants