-
Notifications
You must be signed in to change notification settings - Fork 35.8k
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
util: ParseByteUnits - Parse a string with suffix unit #23249
Conversation
55868c8
to
7d96a50
Compare
Updated with
What I need help with is overflows. I use |
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
933f7ff
to
0d786a1
Compare
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.
Concept ACK. However, I think the implementation can be improved. Why not check the last char first? If it's not a digit then extract it and validate to get the multiplier. Only then parse the number.
0d786a1
to
8b16cb5
Compare
8b16cb5
to
c5d4a59
Compare
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.
Code review and lightly tested ACK c5d4a59. I left some suggestions below, but none are important so feel free to ignore them.
It might be good to add a release note in the doc/
directory mentioning the new argument verification, since in theory it could make a configuration that used to work previously now cause an error on startup.
It would also be good to mention new behavior in the PR description, since current description is a little ambiguous about whether this is only adding a utility function or changing behavior.
c5d4a59
to
f1f8a77
Compare
Added to release docs
Updated PR description. Thanks for feedback @ryanofsky |
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.
Code review ACK f1f8a77. Just suggested changes since last review (thanks!)
6888756
to
f3f8806
Compare
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.
ACK f3f8806
Some styling nits below, which would be good to address before merge.
The style rules are defined in src/.clang-format
. You can use contrib/devtools/clang-format-diff.py
to properly format the patch.
f3f8806
to
6ebb476
Compare
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.
ACK 6ebb476
6ebb476
to
4451a00
Compare
@MarcoFalke would you mind reviewing? Thanks |
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.
ACK 4451a00
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.
Code review ACK 4451a00.
src/util/strencodings.cpp
Outdated
} | ||
|
||
uint64_t num; | ||
if (!ParseUInt64(unit ? str.substr(0, str.size() - 1) : str, &num) || |
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.
With this breaking change (no longer ignoring whitespace), which I think is fine, it might be best to also reject sign characters (+-) and use ToIntegral<uint64_t>
.
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 have tests for +-, whitespace, invalid unit...etc
I'm now using ToIntegral.
I've added an assert(default_multiplier > 0)
but I don't know how to test this. Any ideas examples?
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.
You cannot have the test trigger an assert()
and continue successfully. Instead, you can throw std::out_of_range
from the function and try/catch
it from the test (BOOST_CHECK_THROW()
). But in this case maybe it is best/simpler to just remove the parameter. Then there is no need to test edge cases for it :)
src/util/strencodings.h
Outdated
* @returns optional uint64_t bytes from str or nullopt | ||
* if ParseUInt64 is false, str is empty, trailing whitespace or overflow | ||
*/ | ||
std::optional<uint64_t> ParseByteUnits(const std::string& str, uint64_t default_multiplier = 1024UL * 1024UL); |
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 the default multiplier have a default? It seems like the default is something that is per-arg and not project-wide.
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.
Agreed. I will remove the default.
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.
How about 1
as a default multiplier ?
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.
How about removing the argument? It's not needed. Either way please update doc comment above.
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.
default of 1
seems natural, so my latest change defaults to 1
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.
How about removing the argument? It's not needed.
Indeed! I would remove that unused code. No parameter, no problem :) (what should be its default value? should it have default value? how to test 0
if we have assert()?)
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 do need this multiplier argument. the intent of this function is to be used with maxupload, dbcache...etc. they inheritly have a default multiplier (mostly MiB). the main purpose is maintain backwards capability, e. g. maxupload=300
becomes 300m
.
As I write this, i think I will change the argument to a an enum with the valid units [k|K|m|M...]
with no default value. This makes it clear and easy to verify/test.
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.
@vasild @MarcoFalke @promag
I added an enum ByteUnitMultiplier
to src/util/strencodings.src/util/strencodings.h
Let me know what you think.
4451a00
to
e4e2441
Compare
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.
feel free to ignore the style nits
7f101cc
to
02df252
Compare
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.
Code review ACK 1f51a25. Main changes since last review are removing default multiplier, asserting multiplier is positive, adding enum
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.
The code looks ok (1f51a25). Some nits below, feel free to ignore.
I think the 3 commits should be squashed into one - subsequent ones just change stuff added by the first commit.
A convenience utility for human readable arguments/config e.g. -maxuploadtarget=500g
1f51a25
to
21b58f4
Compare
@MarcoFalke |
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.
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.
Code review ACK 21b58f4. Only changes since last review are dropping optional has_value call, fixing comment punctuation, squashing commits.
BOOST_CHECK_EQUAL(ParseByteUnits("1", noop).value(), 1); | ||
BOOST_CHECK_EQUAL(ParseByteUnits("0", noop).value(), 0); | ||
|
||
BOOST_CHECK_EQUAL(ParseByteUnits("1k", noop).value(), 1000ULL); |
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 also add a test that default_multiplier is ignored completely if passed in via the string? Maybe also add a fuzz test?
This fails to compile on some (unsupported, e.g. cygwin) systems:
|
@rebroad You need a C++17 compiler. |
A convenience utility for parsing human readable strings sizes e.g.
500G
is500 * 1 << 30
The argument/setting
maxuploadtarget
now accept human readable byte units[k|K|m|M|g|G||t|T]
This change backward compatible, defaults to
M
if no unit specified.