-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Fixed compactor tests; Moved to full e2e compact test; Cleaned metrics. #1666
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks right, just want to avoid confusions with the metrics.
Thanks for looking into this metric "swap" @brancz - I am not 100% particularly sure if we won't confuse users, but I think what users was expecting seeing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, some small questions and grammar suggestions (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm 🌮
…s. (#1666) * Fixed compactor tests; Moved to full e2e compact test; Cleaned metrics. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Removed block after each compaction group run. Fixes: #1499 Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Moved to label hash for dir names for compactor groups. Fixes: #1661 Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Addressed comments. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> * Addressed comments, rebased. Signed-off-by: Bartek Plotka <bwplotka@gmail.com> Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
PR is big but I recommend review commit by commit (:
Fixes: #1499
Fixes: #1661
cc @krasi-georgiev as I changed your tests (one was broken, some I moved to proper, full compact_e2e)