-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
[FLINK-15375][core] Improve MemorySize to print / parse with better readability. #10785
[FLINK-15375][core] Improve MemorySize to print / parse with better readability. #10785
Conversation
cc @azagrebin |
Thanks a lot for your contribution to the Apache Flink project. I'm the @flinkbot. I help the community Automated ChecksLast check on commit bfb11ff (Tue Jan 07 07:46:39 UTC 2020) Warnings:
Mention the bot in a comment to re-run the automated checks. Review Progress
Please see the Pull Request Review Guide for a full explanation of the review process. The Bot is tracking the review progress through labels. Labels are applied according to the order of the review items. For consensus, approval by a Flink committer of PMC member is required Bot commandsThe @flinkbot bot supports the following commands:
|
CI report:
Bot commandsThe @flinkbot bot supports the following commands:
|
bfb11ff
to
65aaf3d
Compare
@tillrohrmann The PR is ready for review. Could you take a look at your convenience? |
There was a problem hiding this 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 updating this PR @xintongsong. The changes look good. The only question I had was whether the human readable string should have the format %.3f%s (%d bytes)
instead of %.3f%s (%d)
. What do you think?
.filter(idx -> bytes > ORDERED_UNITS.get(idx).getMultiplier()) | ||
.boxed() | ||
.max(Comparator.naturalOrder()) | ||
.map(idx -> ORDERED_UNITS.get(idx)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could be replaced with lambda method reference.
} else { | ||
double approximate = 1.0 * bytes / highestUnit.getMultiplier(); | ||
return String.format( | ||
"%.3f%s (%d)", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we add %.3f%s (%d bytes)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought people could easily understand that the value in the parentheses are they exact bytes. But true, it would be better to explicitly add the unit.
72f09ab
to
9ac1d96
Compare
@tillrohrmann, thanks for the review. I've addressed your comments. |
Thanks for addressing my comments @xintongsong. Merging this PR now. |
…ing in logs and error messages. This include changes in: - ResourceSpec - ResourceProfile - TaskExecutorResourceSpec - TaskExecutorResourceUtils
…ebibytes. This closes apache#10785.
9ac1d96
to
53d6a43
Compare
…ebibytes. This closes apache#10785.
…ebibytes. This closes #10785.
…ebibytes. This closes apache#10785.
What is the purpose of the change
This PR improves readability and usability of
MemorySize
.Brief change log
MemorySize#toString
.MemorySize#toHumanReadableString
MemorySize#toHumanReadableString
for logs and error messages.Verifying this change
MemorySizeTest
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes)Documentation