Disable chunk write queue by default, allow user to configure the exact size#10425
Conversation
7832211 to
7dadd03
Compare
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
61b47e6 to
80724e5
Compare
codesome
left a comment
There was a problem hiding this comment.
LGTM. @csmarchbanks does the flag name looks fine? Should the flag name also include .experimental.?
Also, the release notes can say that we moved this feature into experimental. (it was not released as experimental before)
cmd/prometheus/main.go
Outdated
| serverOnlyFlag(a, "storage.tsdb.wal-compression", "Compress the tsdb WAL."). | ||
| Hidden().Default("true").BoolVar(&cfg.tsdb.WALCompression) | ||
|
|
||
| serverOnlyFlag(a, "storage.tsdb.head-chunks-write-queue-size", "Size of the queue through which head chunks are written, 0 disables the queue completely. Experimental."). |
There was a problem hiding this comment.
| serverOnlyFlag(a, "storage.tsdb.head-chunks-write-queue-size", "Size of the queue through which head chunks are written, 0 disables the queue completely. Experimental."). | |
| serverOnlyFlag(a, "storage.tsdb.head-chunks-write-queue-size", "[EXPERIMENTAL] Size of the queue through which head chunks are written to the disk to be m-mapped, 0 disables the queue completely."). |
There was a problem hiding this comment.
Perhaps this should be a feature flag instead of a separate flag? That way it is by default experimental and we are signalling that we hope it becomes the default in the future?
There was a problem hiding this comment.
just FYI I copied from here:
prometheus/cmd/prometheus/main.go
Line 357 in 80724e5
and
prometheus/cmd/prometheus/main.go
Line 360 in 80724e5
If there is a better way to signal that something is experimental, maybe we should update these flags too
There was a problem hiding this comment.
I am fine with what is there now. Since it also controls the size I think just marking it as experimental similar to the other commands is reasonable.
There was a problem hiding this comment.
Can we add to the disk to be m-mapped part. (consider this to be a non-blocking comment)
There was a problem hiding this comment.
Can we add to the disk to be m-mapped part. (consider this to be a non-blocking comment)
Done: c8b0a56
Signed-off-by: Mauro Stettler <mauro.stettler@gmail.com>
based on user feedback the chunk write queue had a negative impact on the performance in certain scenarios, so we disable it by default for now. we will try to optimize it further and then maybe enable it by default again in the future.