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

[Improvement]: Add MemorySize for memory value and unit #3329

Open
3 tasks done
Tracked by #3048
klion26 opened this issue Nov 13, 2024 · 2 comments · May be fixed by #3409
Open
3 tasks done
Tracked by #3048

[Improvement]: Add MemorySize for memory value and unit #3329

klion26 opened this issue Nov 13, 2024 · 2 comments · May be fixed by #3409

Comments

@klion26
Copy link
Member

klion26 commented Nov 13, 2024

Search before asking

  • I have searched in the issues and found no similar issues.

What would you like to be improved?

Currently, we'll process memory value and unit manually(et resource configuration in optimizerGroup), and the unit only support m/g, if user configured with mb/gb, we'll get a wrong parsed value.

How should we improve?

Support a MemorySize to parse the memory size with the unit, support the following values: 1, 1k, 1kb, 1kibibytes, etc.

this new one can be compatible with the old one.

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Subtasks

No response

Code of Conduct

@klion26
Copy link
Member Author

klion26 commented Nov 13, 2024

As @baiyangtx you're the original author of FlinkOptimzierContainer#buildOptimizerStartupArgsString what do you think about this? thanks.

@Aireed Aireed added this to the Release 0.8.0 milestone Dec 4, 2024
@Aireed Aireed mentioned this issue Dec 4, 2024
33 tasks
@Jzjsnow Jzjsnow linked a pull request Jan 17, 2025 that will close this issue
3 tasks
@Jzjsnow
Copy link

Jzjsnow commented Jan 17, 2025

In addition to the issues mentioned in the issue above, there is another concern: the default unit for setting flink-conf.jobmanager.memory.process.size and flink-conf.taskmanager.memory.process.size in 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.

In #3409, I add MemorySize to the amoro-common module for parsing flink's memory parameters, consistent with flink's native parsing logic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants