Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

mixin, docs: Rename Compact -> Compactor #2065

Merged
merged 3 commits into from
Jan 28, 2020
Merged

mixin, docs: Rename Compact -> Compactor #2065

merged 3 commits into from
Jan 28, 2020

Conversation

kakkoyun
Copy link
Member

@kakkoyun kakkoyun commented Jan 28, 2020 •
edited
Loading

This PR renames compact to compactor in mixin and docs.

As discussed in #1871

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

Changes

Verification

  • make examples
  • make example-rules-lint
  • make docs
  • make check-docs

@kakkoyun kakkoyun changed the title Rename Compact -> Compactor mixin, docs: Rename Compact -> Compactor Jan 28, 2020
yeya24
yeya24 reviewed Jan 28, 2020
View reviewed changes
examples/alerts/alerts.yaml Outdated
@@ -3,23 +3,23 @@ groups:
rules:
- alert: ThanosCompactorMultipleCompactsAreRunning
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it better to use ThanosCompactorMultipleCompactorsAreRunning

mixin/thanos/alerts/compactor.libsonnet Outdated
@@ -12,7 +12,7 @@
{
alert: 'ThanosCompactorMultipleCompactsAreRunning',
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@kakkoyun
Fix compactor alert name
781fc36 
Signed-off-by: Kemal Akkoyun <[email protected]>
yeya24
yeya24 approved these changes Jan 28, 2020
View reviewed changes
Copy link
Contributor

@yeya24 yeya24 left a comment

Choose a reason for hiding this comment

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

This change LGTM. But I am still confused about what should we change. Should we also rename the metrics like thanos_compact_group_compactions_failures_total ?

@kakkoyun
Copy link
Member Author

@yeya24 From the discussions on the issue, I got the feeling that we won't change metric names yet.
I might be wrong.

@bwplotka What do you think? Is it safe to rename metrics at this stage?

bwplotka
bwplotka approved these changes Jan 28, 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

Yes, it's tricky @yeya24

Ideally we should change it, but it's super distracting change for users as it can fail in hidden way. So for now we postpone metrics, command, and API changes.

@bwplotka bwplotka merged commit d427966 into thanos-io:master Jan 28, 2020
@kakkoyun kakkoyun deleted the mixin_compactor branch January 28, 2020 15:22
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

@yeya24 yeya24 yeya24 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.
3 participants
@kakkoyun @bwplotka @yeya24