strutil: add new ParseByteSize #5719

mvo5 · 2018-08-24T10:45:16Z

The new ParseValueWithUnit helper allows the user to write
"500kB" which will be converted into 500*1000. This will be
used in the rate-limit PR to support options like:

snap set core refresh.rate-limit=500kB

Split out to make the review simpler.

zyga

LGTM with one open question about kB vs KB

zyga · 2018-08-24T10:48:14Z

strutil/strutil.go

+func ParseValueWithUnit(inp string) (int64, error) {
+	unitMultiplier := map[string]int{
+		"B":  1,
+		"kB": 1000,


Should we support KB in case someone just types that out of a habit? (regardless if they meant 1024 or 1000)

I meant that lower case and upper case difference is pretty subtle and may be missed by people. For usability we should perhaps ignore case here.

Yeah, I was sitting on the fence about it but I think it does make sense to make the users life easier this way. I will make it ignore the case.

zyga · 2018-08-24T10:51:04Z

strutil/strutil_test.go

+		{"1MB", 1000 * 1000},
+		{"20MB", 20 * 1000 * 1000},
+		{"1GB", 1000 * 1000 * 1000},
+		{"31GB", 31 * 1000 * 1000 * 1000},


Hard drive vendors approve of this number ;-)

stolowski

Thanks, this looks very nice and useful. One request for an extra test checks, pluse some suggestions/questions.

stolowski · 2018-08-24T10:51:23Z

strutil/strutil.go

+		"GB": 1000 * 1000 * 1000,
+		"TB": 1000 * 1000 * 1000 * 1000,
+		"PB": 1000 * 1000 * 1000 * 1000 * 1000,
+		"EB": 1000 * 1000 * 1000 * 1000 * 1000 * 1000,


You got carried away, did't you? 😄 But that's fine.

stolowski · 2018-08-24T10:52:21Z

strutil/strutil.go

+		"PB": 1000 * 1000 * 1000 * 1000 * 1000,
+		"EB": 1000 * 1000 * 1000 * 1000 * 1000 * 1000,
+	}
+	if len(inp) < 2 {


Would it make sense to default to bytes if unit is omited, to simplify calling side? That of course means unit won't be mandatory, but I guess that's the case since we already have values around in the state without "B" suffix.

My gut feeling is that we should enforce a unit. The error message tries to help steer the user into the right direction.

stolowski · 2018-08-24T11:03:54Z

strutil/strutil.go

+		return 0, fmt.Errorf("cannot parse %q: need a unit", inp)
+	}
+	// simple case, "1234B", read as byte
+	if lastCh := inp[len(inp)-1]; lastCh == 'B' {


Unless I'm missing something, we will always hit this case since every unit ends with 'B', so we will (usually) unnecessarily try convert the string to int, it will fail so we will carry on with two-char suffix check.
Since two-letter units will be used most of the time I suppose, would it make sense to reverse and re-organise the two conditions (unitMultiplier[unit] lookup failure wouldn't be fatal in such case), or alternatively enhance this one to check if the inp[len(inp)-2] character is a digit to avoid ParseInt?

Yes but then we don't do anything when err != nil (we just continue and then hit the remaining cases)

Yes, what I'm trying to say is we will hit ParseInt error case 99% of the time if we check for 'B' suffix first.

And to be clear, the logic as currently is is perfectly correct, just suggesting to slightly re-organize the checks so that we only attempt ParseInt when we know unit is already correct (after two-character lookup, and then 'B' suffix check).

I changed the order of the checks now.

stolowski · 2018-08-24T11:07:06Z

strutil/strutil_test.go

+		{"1MB", 1000 * 1000},
+		{"20MB", 20 * 1000 * 1000},
+		{"1GB", 1000 * 1000 * 1000},
+		{"31GB", 31 * 1000 * 1000 * 1000},


Can you add TB/EB/PB checks as well?

The new ParseValueWithUnit helper allows the user to write "500kB" which will be converted into 500*1000. This will be used in the rate-limit PR to support options like: snap set core refresh.rate-limit=500kB Split out to make the review simpler.

bboozzoo · 2018-08-24T10:56:13Z

strutil/strutil.go

@@ -111,3 +111,37 @@ func TruncateOutput(data []byte, maxLines, maxBytes int) []byte {
 	}
 	return data
 }
+
+// ParseValueWithUnit parses a value like 500kB and returns the number
+// in byte


bboozzoo · 2018-08-24T14:10:26Z

strutil/strutil.go

+	}
+	// two char suffix
+	unit := inp[len(inp)-2:]
+	mul, ok := unitMultiplier[unit]


Or the old-school method and you can drop the map:

val := uint64(123) unit := "GB" units := []string{"B", "KB", "MB", "GB", "TB", "PB"} for i := 0; i < len(units) && unit != units[i]; i++ { val *= 1000 }

codecov-io · 2018-08-24T15:09:45Z

Codecov Report

Merging #5719 into master will increase coverage by <.01%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #5719      +/-   ##
==========================================
+ Coverage   78.97%   78.98%   +<.01%     
==========================================
  Files         524      524              
  Lines       40007    40029      +22     
==========================================
+ Hits        31596    31616      +20     
- Misses       5843     5844       +1     
- Partials     2568     2569       +1

Impacted Files	Coverage Δ
strutil/strutil.go	`95.23% <90.9%> (-2.33%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0ca9c70...3002927. Read the comment docs.

stolowski

Thank you for the change! +1

codecov-io · 2018-08-27T11:49:38Z

Codecov Report

Merging #5719 into master will increase coverage by 0.01%.
The diff coverage is 87.09%.

@@            Coverage Diff             @@
##           master    #5719      +/-   ##
==========================================
+ Coverage   78.96%   78.98%   +0.01%     
==========================================
  Files         523      525       +2     
  Lines       40033    40073      +40     
==========================================
+ Hits        31613    31651      +38     
- Misses       5849     5850       +1     
- Partials     2571     2572       +1

Impacted Files	Coverage Δ
strutil/strutil.go	`93.05% <87.09%> (-4.51%)`	⬇️
overlord/ifacestate/handlers.go	`63.57% <0%> (-0.32%)`	⬇️
httputil/redirect18.go	`100% <0%> (ø)`
httputil/transport17.go	`100% <0%> (ø)`
wrappers/binaries.go	`83.33% <0%> (+5.55%)`	⬆️
httputil/useragent.go	`82.85% <0%> (+5.71%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4cbc187...8f5c0c4. Read the comment docs.

niemeyer

Couple of suggestions, and LGTM.

niemeyer · 2018-08-27T12:56:57Z

strutil/strutil.go

+
+// ParseValueWithUnit parses a value like 500kB and returns the number
+// in bytes. The case of the unit will be ignored for user convenience.
+func ParseValueWithUnit(inp string) (int64, error) {


The function name doesn't give a hint about what kind of value we have at hand (time, distance, length, etc). Perhaps ParseByteSize or ParseByteUnit or ParseSizeUnit?

niemeyer · 2018-08-27T12:59:40Z

strutil/strutil.go

+	}
+	asByte, err := strconv.ParseInt(inp[:len(inp)-1], 10, 64)
+	if err != nil {
+		return 0, err


This error will probably look awkward with a string like "200GiB".

Nice catch, will fix.

Rename strutil.ParseValueWithUnit to strutil.ParseByteSize and improve error handling for cases like "200GiB" (and add extra tests). Thanks to Gustavo.

zyga approved these changes Aug 24, 2018

View reviewed changes

stolowski suggested changes Aug 24, 2018

View reviewed changes

mvo5 mentioned this pull request Aug 24, 2018

many: add refresh.rate-limit core option #5606

Merged

strutil: add new ParseValueWithUnit

3002927

The new ParseValueWithUnit helper allows the user to write "500kB" which will be converted into 500*1000. This will be used in the rate-limit PR to support options like: snap set core refresh.rate-limit=500kB Split out to make the review simpler.

mvo5 force-pushed the parseUnit branch from 637a712 to 3002927 Compare August 24, 2018 11:24

bboozzoo approved these changes Aug 24, 2018

View reviewed changes

mvo5 added 4 commits August 27, 2018 08:40

strutil: ignore the case of the unit (kb,kB,KB are all the same now)

9ef590f

address review feedback

eadf56f

strutil: check the common case with units with two chars first

3f18100

Merge remote-tracking branch 'upstream/master' into parseUnit

99e2c1a

stolowski approved these changes Aug 27, 2018

View reviewed changes

niemeyer approved these changes Aug 27, 2018

View reviewed changes

strutil: address review feedback for ParseByteSize

8f5c0c4

Rename strutil.ParseValueWithUnit to strutil.ParseByteSize and improve error handling for cases like "200GiB" (and add extra tests). Thanks to Gustavo.

mvo5 force-pushed the parseUnit branch from f6ced98 to 8f5c0c4 Compare August 28, 2018 05:43

mvo5 changed the title ~~strutil: add new ParseValueWithUnit~~ strutil: add new ParseByteSize Aug 28, 2018

mvo5 added 2 commits August 28, 2018 08:38

strutil: update test names

a9d46be

Merge remote-tracking branch 'upstream/master' into parseUnit

dded981

mvo5 merged commit db72d85 into snapcore:master Aug 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

strutil: add new ParseByteSize #5719

strutil: add new ParseByteSize #5719

mvo5 commented Aug 24, 2018

zyga left a comment

zyga Aug 24, 2018

bboozzoo Aug 24, 2018

zyga Aug 24, 2018

mvo5 Aug 27, 2018

zyga Aug 24, 2018

stolowski left a comment

stolowski Aug 24, 2018

stolowski Aug 24, 2018

mvo5 Aug 27, 2018

stolowski Aug 24, 2018

zyga Aug 24, 2018

stolowski Aug 24, 2018

stolowski Aug 24, 2018

mvo5 Aug 27, 2018

stolowski Aug 24, 2018

bboozzoo Aug 24, 2018

bboozzoo Aug 24, 2018

codecov-io commented Aug 24, 2018

stolowski left a comment

codecov-io commented Aug 27, 2018 •

edited

niemeyer left a comment

niemeyer Aug 27, 2018

niemeyer Aug 27, 2018

mvo5 Aug 27, 2018

strutil: add new ParseByteSize #5719

strutil: add new ParseByteSize #5719

Conversation

mvo5 commented Aug 24, 2018

zyga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stolowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 24, 2018

Codecov Report

stolowski left a comment

Choose a reason for hiding this comment

codecov-io commented Aug 27, 2018 • edited

Codecov Report

niemeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 27, 2018 •

edited