-
Notifications
You must be signed in to change notification settings - Fork 812
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
Add shuffle-sharding for the compactor #4433
Merged
Merged
Changes from all commits
Commits
Show all changes
17 commits
Select commit
Hold shift + click to select a range
776bbce
Add metrics for remaining planned compactions
ac1214 e8881fb
fix unit tests
ac1214 0da3e9b
Add shuffle sharding for compactor
ac1214 acb1866
update changelog
ac1214 2063e96
fix linting
ac1214 1c8d4d8
Fix build errors
alvinlin123 23e79a0
Fix up change log
alvinlin123 eafb14e
Fix linting error
alvinlin123 fb15202
Remove use of nolint
alvinlin123 ccbf001
Compactor.ownUser now determines whether the user is owned by a compa…
roystchiang 3cb44ee
fix bug where multiple compactors are trying to cleanup the same tena…
roystchiang dd9681b
set all remaining compation in one go, instead of slowly incrementing…
roystchiang 3ea1e36
rename ownUser function for better readability
roystchiang 1a7f65a
address comments
roystchiang 3cd209b
fixed rebase issues
roystchiang 18a8e70
fix tests
roystchiang 137a313
Merge branch 'master' into add-sharding
alanprot File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Every Compactor now "owns" all the users, even if it doesn't actually compact any blocks for most users. One particular impact I saw is a huge growth of metadata syncs. Instead, maybe we can compute
ownUser
based on the shuffle shard of a tenant.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.
Quick fix that seems to work: b0c2c2a
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.
hey @edma2 thanks for taking a look at the compactor PR. We are actively testing this branch in our beta environment, and I'm glad to hear that it is working for you.
We have a similar fix to your commit, and I can confirm that we also saw an improvement in the metadata syncs.
However, on the tenant clean up side, we were running into issues where the deleted tenant directory was left dangling. While compactor-A is deleting the deletion markers, compactor-B is also trying to sync the data, and re-uploads the block index. We currently provide an override for the tenant shard size on the clean-up path so that only 1 compactor owns the cleanup for a given tenant.
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.
thanks for the tip on the cleanup side. Makes sense that you don't want multiple compactors repeating the same work and potentially conflicting.