Skip to content

Add a wal compression flag in rule and receive#1933

Merged
bwplotka merged 1 commit intothanos-io:masterfrom
yeya24:add-wal-compress
Jan 8, 2020
Merged

Add a wal compression flag in rule and receive#1933
bwplotka merged 1 commit intothanos-io:masterfrom
yeya24:add-wal-compress

Conversation

@yeya24
Copy link
Contributor

@yeya24 yeya24 commented Jan 2, 2020 •
edited
Loading

Signed-off-by: yeya24 yb532204897@gmail.com

  • I added CHANGELOG entry for this change.
  • Change is not relevant to the end user.

Changes

Add a WAL compression option in rule and receive

Verification

bwplotka
bwplotka reviewed Jan 3, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, I think it should be under flag, what do you think?

@yeya24
Copy link
Contributor Author

yeya24 commented Jan 3, 2020

Should it be a flag? Receiver sets this option to true by default. Is there any use case that people want to set it to false?

@bwplotka
Copy link
Member

bwplotka commented Jan 3, 2020 •
edited
Loading

It is an option in Prometheus, plus false by default, so I would stick to the same here. You are right for the receiver - we might want the option as well.

@yeya24
Copy link
Contributor Author

yeya24 commented Jan 3, 2020 •
edited
Loading

OK. I will add the flag for ruler and receiver in this PR.

@yeya24
Copy link
Contributor Author

yeya24 commented Jan 4, 2020 •
edited
Loading

I have added a flag for rule and receive. Should I add the changelog?

@squat
Copy link
Member

squat commented Jan 4, 2020

Definitely worth mentioning in changelog if the default has changed

@yeya24 yeya24 changed the title Add wal compression in rule Add a wal compression flag in rule and receive Jan 4, 2020
@brancz
Copy link
Member

brancz commented Jan 6, 2020

WAL compression is intended to become the default in Prometheus, we just can't do it in Prometheus yet as that would be too breaking of a change. I personally think we should just always have WAL compression enabled, the CPU footprint of receive is so tiny it's the right trade-off in my opinion.

tl;dr I think we should leave things as they are in this regard.

@bwplotka
Copy link
Member

bwplotka commented Jan 6, 2020

Should we make it true by default then?

@brancz
Copy link
Member

brancz commented Jan 6, 2020

I'm not sure this should be configurable at all, but yes if this config option exists then it should definitely be true by default.

bwplotka
bwplotka approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@bwplotka bwplotka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after comments (:

cmd/thanos/receive.go Outdated

tsdbBlockDuration := modelDuration(cmd.Flag("tsdb.block-duration", "Duration for local TSDB blocks").Default("2h").Hidden())

walCompression := cmd.Flag("tsdb.wal-compression", "Compress the tsdb WAL.").Default("false").Bool()
Copy link
Member

@bwplotka bwplotka Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's have true by default then

Copy link
Contributor Author

@yeya24 yeya24 Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

Copy link
Member

@bwplotka bwplotka Jan 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We thank you (:

@yeya24
add wal compression option in rule and receive
7543bdc 
Signed-off-by: yeya24 <yb532204897@gmail.com>
@bwplotka bwplotka merged commit 36f7536 into thanos-io:master Jan 8, 2020
@yeya24 yeya24 deleted the add-wal-compress branch January 8, 2020 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bwplotka bwplotka bwplotka approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yeya24 @bwplotka @squat @brancz