Add configuration parameter to expose rate limit for TSM compaction. #9939

zachgoldstein · 2018-06-04T19:48:35Z

Closes: 9938

Required for all non-trivial PRs

Sign CLA (if not already signed)

Required only if applicable

You can erase any checkboxes below this note if they are not applicable to your Pull Request.

Config changes: update sample config (etc/config.sample.toml), server NewDemoConfig method, and Diagnostics methods reporting config settings, if necessary
InfluxData Documentation: issue filed or pull request submitted <link to issue or pull request>

jacobmarble

@zachgoldstein the compaction throughput feature has been helpful for some InfluxDB and Enterprise users that I'm aware of, so thanks for exposing it better in this PR. You followed existing patterns, which made this a very straightforward review.

Because it took us so long to get around to this review, the PR won't merge, so I'm sorry to ask you to rebase against master, in addition to my code comments. Please let me know if you can't get to that right away and I'll find some time later this week to do it for you.

If you are able to rebase and follow-up to my review, then I'll build and test locally before merging.

jacobmarble · 2018-07-18T23:06:02Z

etc/config.sample.toml

@@ -97,6 +97,15 @@
  # to cache snapshotting.
  # max-concurrent-compactions = 0

+  # CompactThroughput is the rate limit in bytes per second that we
+  # will allow TSM compactions to happen. Note that short bursts are allowed


The feature exists to prevent long IO blocking, so instead of "compaction to happen." how about "compactions to write to disk.".

jacobmarble · 2018-07-18T23:08:03Z

tsdb/config.go

@@ -37,6 +37,17 @@ const (
 	// will compact all TSM files in a shard if it hasn't received a write or delete
 	DefaultCompactFullWriteColdDuration = time.Duration(4 * time.Hour)

+	// DefaultCompactThroughput is the rate limit in bytes per second that we
+	// will allow TSM compactions to happen. Not that short bursts are allowed
+	// to happen at a possibly larger value, set by DefaultCompactBurstThroughput.


Same as above wrt "happen".

jacobmarble · 2018-07-18T23:08:26Z

etc/config.sample.toml

+  # compact-throughput = "48m"
+
+  # CompactBurstThroughput is the rate limit in bytes per second that we
+  # will allow TSM compactions to happen.


Same as above, replace "happen" with clearer description.

jacobmarble · 2018-07-18T23:08:41Z

tsdb/config.go

+	DefaultCompactThroughput = 0
+
+	// DefaultCompactBurstThroughput is the rate limit in bytes per second that we
+	// will allow TSM compactions to happen. If this is not set, the burst value


Same as above wrt "happen".

jacobmarble · 2018-07-18T23:09:43Z

tsdb/store.go

+	throughput := int(s.EngineOptions.Config.CompactThroughput)
+	burstThroughput := int(s.EngineOptions.Config.CompactBurstThroughput)
+	if throughput > 0 {
+		if burstThroughput == 0 {


Don't want burst < throughput, so how about this?

if burstThroughput < throughput {

jacobmarble · 2018-07-18T23:13:21Z

tsdb/store.go

+		s.Logger.Info(
+			"Compaction throughput enabled",
+			zap.String("throughput", fmt.Sprintf("%d%%", throughput)),
+			zap.String("throughput_burst", fmt.Sprintf("%d%%", burstThroughput)),


Here, it's called "throughput_burst" but elsewhere in the PR it's "burstThroughput" etc. Please select one ordering.

FWIW, I prefer the "throughput burst" ordering because it keeps the config keys "compact-throughput" and "compact-throughput-burst" together visually and lexicographically.

zachgoldstein · 2018-07-19T03:45:45Z

@jacobmarble Great! Excited to see this one move forward. I've rebased against master and made those changes. Let me know if there's anything else to do.

I see the AppVeyor build failed as well, though I'm not sure how it could be related to changes in this PR. I see a message saying: unable to deduce repository and source type for "collectd.org".

jacobmarble · 2018-07-19T15:38:28Z

@zachgoldstein thanks, I'll take a look now. I just restarted the AppVeyor failure, I think it was a transient network failure.

Can you describe your use case? Will this feature fix a problem for your employer?

jacobmarble · 2018-07-19T15:49:02Z

@zachgoldstein please run gofmt against the change:

# github.com/influxdata/influxdb/tsdb
tsdb\store.go:239:64: syntax error: unexpected newline, expecting comma or )
tsdb\store.go:258:64: syntax error: unexpected newline, expecting comma or )

zachgoldstein · 2018-07-19T15:54:38Z

@jacobmarble I'm a freelancer, so in a previous contract I was helping to build out some financial analysis software. We chose to use InfluxDB as the main store for market securities data and bumped into an issue where the machine running influx would continually cycle between consuming all the machine's memory, crashing, and then restarting. I outlined a bit of this in #9938. For us, the issue was severe enough that it was close to being a dealbreaker in using InfluxDB. At the time we worked around the issue by making our shard size much smaller, which seemed to really cut memory consumption, but migrating to that was quite time consuming. Once the contract finished, I looked into the issue a bit more deeply, tracked it down to a TSM compaction kicking off, and then submitted the issue and this PR.

So ya, I'm not fixing any immediate issue I'm experiencing, just trying to address something I bumped into before and help move things forward.

Gah! Sorry fixed up the formatting just now.

zachgoldstein · 2018-07-30T19:56:27Z

@jacobmarble Is there anything else here you'd like me to fix up?

jacobmarble

Sorry, I went on vacation, thought we had tied this up. Here's a comment left pending for two weeks.

jacobmarble · 2018-07-19T22:31:05Z

tsdb/config.go

+	// will allow TSM compactions to write to disk. Not that short bursts are allowed
+	// to happen at a possibly larger value, set by DefaultCompactThroughputBurst.
+	// A value of 0 here will disable compaction rate limiting
+	DefaultCompactThroughput = 0


Let's keep the default where it was, at 48 * 1024 * 1024, for both throughput and burst. As before, we want the user to explicitly set zero for unlimited.

Closes: 9938

zachgoldstein · 2018-08-07T14:06:35Z

@jacobmarble Gotcha no worries! Totally missed that one comment, fixed it up and rebased now.

jacobmarble · 2018-08-07T16:23:18Z

@zachgoldstein thanks again for this!

mjiderhamn · 2021-07-28T08:12:14Z

Are there any recommendations for how to come up with a sensible value for this setting, or is it only trial and error for your particular set of data and hardware?

ghost added the proposed label Jun 4, 2018

timhallinflux requested review from e-dard and jacobmarble July 18, 2018 22:44

jacobmarble requested changes Jul 18, 2018

View reviewed changes

jacobmarble self-assigned this Jul 18, 2018

zachgoldstein force-pushed the master branch 3 times, most recently from 9736834 to 0b75f2d Compare July 19, 2018 03:42

zachgoldstein force-pushed the master branch from 0b75f2d to dba0c51 Compare July 19, 2018 15:55

jacobmarble added kind/bug kind/feature-request and removed kind/bug labels Jul 19, 2018

timhallinflux mentioned this pull request Jul 24, 2018

Document Compaction Throughput Config Option influxdata/docs.influxdata.com-ARCHIVE#1737

Closed

jacobmarble reviewed Aug 3, 2018

View reviewed changes

Add configuration parameter to expose rate limit for TSM compaction.

0ef3752

Closes: 9938

zachgoldstein force-pushed the master branch from dba0c51 to 0ef3752 Compare August 7, 2018 14:05

jacobmarble approved these changes Aug 7, 2018

View reviewed changes

jacobmarble merged commit 0348221 into influxdata:master Aug 7, 2018

ghost removed the proposed label Aug 7, 2018

jacobmarble mentioned this pull request Aug 7, 2018

tsdb: Cleanup compaction throughput code #10166

Merged

timhallinflux added this to the 1.7.0 milestone Nov 5, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add configuration parameter to expose rate limit for TSM compaction. #9939

Add configuration parameter to expose rate limit for TSM compaction. #9939

zachgoldstein commented Jun 4, 2018 •

edited

Loading

jacobmarble left a comment

jacobmarble Jul 18, 2018 •

edited

Loading

jacobmarble Jul 18, 2018

jacobmarble Jul 18, 2018

jacobmarble Jul 18, 2018

jacobmarble Jul 18, 2018 •

edited

Loading

jacobmarble Jul 18, 2018

zachgoldstein commented Jul 19, 2018

jacobmarble commented Jul 19, 2018

jacobmarble commented Jul 19, 2018

zachgoldstein commented Jul 19, 2018

zachgoldstein commented Jul 30, 2018

jacobmarble left a comment

jacobmarble Jul 19, 2018

zachgoldstein commented Aug 7, 2018

jacobmarble commented Aug 7, 2018

mjiderhamn commented Jul 28, 2021

Add configuration parameter to expose rate limit for TSM compaction. #9939

Add configuration parameter to expose rate limit for TSM compaction. #9939

Conversation

zachgoldstein commented Jun 4, 2018 • edited Loading

Required for all non-trivial PRs

Required only if applicable

jacobmarble left a comment

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

jacobmarble Jul 18, 2018

Choose a reason for hiding this comment

zachgoldstein commented Jul 19, 2018

jacobmarble commented Jul 19, 2018

jacobmarble commented Jul 19, 2018

zachgoldstein commented Jul 19, 2018

zachgoldstein commented Jul 30, 2018

jacobmarble left a comment

Choose a reason for hiding this comment

jacobmarble Jul 19, 2018

Choose a reason for hiding this comment

zachgoldstein commented Aug 7, 2018

jacobmarble commented Aug 7, 2018

mjiderhamn commented Jul 28, 2021

zachgoldstein commented Jun 4, 2018 •

edited

Loading

jacobmarble Jul 18, 2018 •

edited

Loading

jacobmarble Jul 18, 2018 •

edited

Loading