Allow human-readable byte sizes in config #8891

andrewhare · 2017-09-27T22:33:44Z

Update support in the toml package for parsing human-readble byte sizes.
Supported size suffixes are "k" or "K" for kibibytes, "m" or "M" for
mebibytes, and "g" or "G" for gibibytes. If a size suffix isn't specified
then bytes are assumed.

In the config, cache-max-memory-size and cache-snapshot-memory-size are
now typed as toml.Size and support the new syntax.

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated

Required only if applicable

Provide example syntax
Config changes: update sample config (etc/config.sample.toml), server NewDemoConfig method, and Diagnostics methods reporting config settings, if necessary

jsternberg · 2017-10-03T17:01:47Z

toml/toml.go

-	default:
-		return fmt.Errorf("unknown size suffix: %c", suffix)
+	if maxInt/mult < size {
+		return fmt.Errorf("size would overflow the max size (%d) of an int: %s", maxInt, string(text))


This should say a uint because now it's an unsigned integer. I don't think we need to specify it is a uint64. Can you also use math.MaxUint64 for the size check? It would make it so maxInt isn't needed anymore.

maxInt is actually based on the largest value of int, and Size was previously defined as the type int. If the goal is for Size to be a uint64, then use MaxUint64. If it's meant to be int, then keep the current constant and change the underlying type of Size.

jsternberg · 2017-10-03T17:05:03Z

toml/toml.go

 	// Check for overflow.
 	if size > maxInt {
-		return fmt.Errorf("size %d cannot be represented by an int", size)
+		return fmt.Errorf("size %d is too large", size)


I don't think this check is possible. I don't think the previous code worked and I don't see any tests that verified this if statement ever worked.

I agree, the previous check should cover it since we're just dealing with known powers of two (shifting).

jsternberg · 2017-10-03T17:06:58Z

etc/config.sample.toml

@@ -71,10 +71,12 @@

  # CacheMaxMemorySize is the maximum size a shard's cache can
  # reach before it starts rejecting writes.
+  # Values without a size suffix (e.g. 30k, 20m, 10g) are in bytes.


Can you update this to say: Valid size suffixes are k, m, or g (case insensitive). Values without a size suffix are in bytes.

joelegasse · 2017-10-10T17:50:59Z

toml/toml.go

 	// Parse numeric portion of value.
-	length := len(string(text))
-	size, err := strconv.ParseInt(string(text[:length-1]), 10, 64)
+	size, err := strconv.ParseInt(string(sizeText), 10, 64)


This should be probably be ParseUint, so we don't end up with negative sizes

joelegasse · 2017-10-10T18:32:47Z

toml/toml.go

-	default:
-		return fmt.Errorf("unknown size suffix: %c", suffix)
+	if maxInt/mult < size {
+		return fmt.Errorf("size would overflow the max size (%d) of an int: %s", maxInt, string(text))


maxInt is actually based on the largest value of int, and Size was previously defined as the type int. If the goal is for Size to be a uint64, then use MaxUint64. If it's meant to be int, then keep the current constant and change the underlying type of Size.

joelegasse · 2017-10-10T18:53:34Z

toml/toml.go

 	// Check for overflow.
 	if size > maxInt {
-		return fmt.Errorf("size %d cannot be represented by an int", size)
+		return fmt.Errorf("size %d is too large", size)


I agree, the previous check should cover it since we're just dealing with known powers of two (shifting).

Update support in the `toml` package for parsing human-readble byte sizes. Supported size suffixes are "k" or "K" for kibibytes, "m" or "M" for mebibytes, and "g" or "G" for gibibytes. If a size suffix isn't specified then bytes are assumed. In the config, `cache-max-memory-size` and `cache-snapshot-memory-size` are now typed as `toml.Size` and support the new syntax.

jsternberg · 2017-11-01T16:10:27Z

@andrewhare @joelegasse I implemented the feedback to this so we can get it in for 1.4. Can you two check to make sure that there isn't anything I missed?

mark-rushakoff

A couple copy changes and one new test requested, then LGTM.

mark-rushakoff · 2017-11-01T16:21:07Z

etc/config.sample.toml

@@ -71,10 +71,14 @@

  # CacheMaxMemorySize is the maximum size a shard's cache can
  # reach before it starts rejecting writes.
+  # Valid size suffixes are k, m, or g (case insensitive).
+  # Values without a size suffix (e.g. 30k, 20m, 10g) are in bytes.


"without a size suffix" - should it be 30, 20, 10 in that case? Or maybe just say that all values represent bytes.

To reduce any other confusion, it would help if this comment clarified k means 1000 and not 1024.

I'm not sure that's the case. Since the sizes are byte shifts, then 1 << 10 is 1024. I'll see if I can clarify this comment though since what it's trying to say is that 30k, 20m, and 10g have size suffixes.

You're right, it's 1024 and not 1000. Should still be clarified for users.

I updated the configuration file to include a note that 1024 = 1k. That should be good enough for someone to extrapolate that these are 1024 and not 1000.

mark-rushakoff · 2017-11-01T16:23:38Z

toml/toml.go

+		case 'g', 'G':
+			mult = 1 << 30 // GiB
+		default:
+			return fmt.Errorf("unknown size suffix: %c", suffix)


It would be helpful if this error mentioned that "valid suffixes are k, m, g".

mark-rushakoff · 2017-11-01T16:25:36Z

tsdb/config.go

@@ -71,8 +71,8 @@ type Config struct {
 	QueryLogEnabled bool `toml:"query-log-enabled"`

 	// Compaction options for tsm1 (descriptions above with defaults)
-	CacheMaxMemorySize             uint64        `toml:"cache-max-memory-size"`
-	CacheSnapshotMemorySize        uint64        `toml:"cache-snapshot-memory-size"`
+	CacheMaxMemorySize             toml.Size     `toml:"cache-max-memory-size"`


I'd like to see an addition to tsdb/config_test.go that sets one of these new toml.Size values to something with a suffix.

jsternberg · 2017-11-01T18:02:46Z

etc/config.sample.toml

-  # cache-max-memory-size = 1048576000
+  # Valid size suffixes are k, m, or g (case insensitive).
+  # Vaues without a size suffix are in bytes.
+  # cache-max-memory-size = "1g"


Just a note for those who might check this, the sample configuration file has the wrong value. The code has 1 GB as the default and 1 GB in bytes is 1073741824 bytes, not 1048576000.

I see, the old sample config value was 1000m but the const DefaultCacheMaxMemorySize is actually 1g (i.e. 1024m). Fixing the sample config to 1g makes sense here.

mark-rushakoff · 2017-11-01T18:07:16Z

etc/config.sample.toml

-  # cache-max-memory-size = 1048576000
+  # Valid size suffixes are k, m, or g (case insensitive).
+  # Vaues without a size suffix are in bytes.
+  # cache-max-memory-size = "1g"


I see, the old sample config value was 1000m but the const DefaultCacheMaxMemorySize is actually 1g (i.e. 1024m). Fixing the sample config to 1g makes sense here.

jsternberg · 2017-11-02T18:34:43Z

The circle tests continue to fail for some reason, but the appveyor ones succeeded and the ones that failed, the race tests, seem to work on my computer and have little to nothing to do with this PR. I'm going to merge this since I don't think the unit tests are actual failures.

andrewhare requested a review from joelegasse September 27, 2017 22:33

ghost assigned andrewhare Sep 27, 2017

ghost added the review label Sep 27, 2017

jsternberg suggested changes Oct 3, 2017

View reviewed changes

joelegasse reviewed Oct 10, 2017

View reviewed changes

jsternberg force-pushed the amh-human-readable-sizes branch from d69a1a6 to 978094f Compare November 1, 2017 16:09

ghost assigned jsternberg Nov 1, 2017

jsternberg approved these changes Nov 1, 2017

View reviewed changes

mark-rushakoff suggested changes Nov 1, 2017

View reviewed changes

jsternberg force-pushed the amh-human-readable-sizes branch from 978094f to 9f59680 Compare November 1, 2017 17:51

jsternberg reviewed Nov 1, 2017

View reviewed changes

Implement pull request feedback for human readable sizes

87ed89e

jsternberg force-pushed the amh-human-readable-sizes branch from 9f59680 to 87ed89e Compare November 1, 2017 18:08

mark-rushakoff approved these changes Nov 1, 2017

View reviewed changes

jsternberg merged commit a8a36d1 into master Nov 2, 2017

ghost removed the review label Nov 2, 2017

jsternberg deleted the amh-human-readable-sizes branch November 2, 2017 18:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow human-readable byte sizes in config #8891

Allow human-readable byte sizes in config #8891

andrewhare commented Sep 27, 2017 •

edited

Loading

jsternberg Oct 3, 2017

joelegasse Oct 10, 2017

jsternberg Oct 3, 2017

joelegasse Oct 10, 2017

jsternberg Oct 3, 2017

joelegasse Oct 10, 2017

joelegasse Oct 10, 2017

joelegasse Oct 10, 2017

jsternberg commented Nov 1, 2017 •

edited

Loading

mark-rushakoff left a comment

mark-rushakoff Nov 1, 2017

jsternberg Nov 1, 2017

mark-rushakoff Nov 1, 2017

jsternberg Nov 1, 2017

mark-rushakoff Nov 1, 2017

jsternberg Nov 1, 2017

mark-rushakoff Nov 1, 2017

jsternberg Nov 1, 2017

jsternberg Nov 1, 2017

mark-rushakoff Nov 1, 2017

mark-rushakoff Nov 1, 2017

jsternberg commented Nov 2, 2017

Allow human-readable byte sizes in config #8891

Allow human-readable byte sizes in config #8891

Conversation

andrewhare commented Sep 27, 2017 • edited Loading

Required for all non-trivial PRs

Required only if applicable

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

jsternberg commented Nov 1, 2017 • edited Loading

mark-rushakoff 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

jsternberg commented Nov 2, 2017

andrewhare commented Sep 27, 2017 •

edited

Loading

jsternberg commented Nov 1, 2017 •

edited

Loading