Compactor: do not issue object storage HEAD API calls to check if cached meta.json files still exist #5063
Conversation
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
pracucci
force-pushed
the
remove-metafetcher-exists-call
branch
from
0b19e02 to
075f1de
Compare
This was referenced May 24, 2023
pracucci
force-pushed
the
remove-metafetcher-exists-call
branch
3 times, most recently
from
3ac2200 to
6f3dba5
Compare
…eta.json Signed-off-by: Marco Pracucci <[email protected]>
pracucci
force-pushed
the
remove-metafetcher-exists-call
branch
from
6f3dba5 to
8e4b3e3
Compare
Signed-off-by: Marco Pracucci <[email protected]>
pstibrany
approved these changes
May 25, 2023
| @@ -314,6 +343,12 @@ func (f *MetaFetcher) fetchMetadata(ctx context.Context) (interface{}, error) { | |||
| return nil | |||
| } | |||
|
|
|||
| // If requested, skip any block marked for deletion. | |||
| if _, marked := markedForDeletion[id]; excludeMarkedForDeletion && marked { | |||
There was a problem hiding this comment.
nit: we don't need to check for excludeMarkedForDeletion again. If it's false then markedForDeletion will be nil (ie. empty map).
There was a problem hiding this comment.
The reason why I added the check here as well is to make the code more obvious when reading it. Given it doesn't add extra complexity, I would keep it.
Signed-off-by: Marco Pracucci <[email protected]>
Co-authored-by: Peter Štibraný <[email protected]>
|
I completed the manual tests on this PR. Going to merge it. |
3 tasks
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this PR does
The compactor issue a "object exists" API call for every block of each tenant every
-compactor.compaction-interval. If you run a Mimir cluster with a large number of tenants (order of tens of thousands) and 1 year blocks retention, this can causes a very high number of API calls (order of hundred of millions per day). This PR removes such call.The rationale of the change introduced in this PR is better explained here, but can be summarised as:
Exists()on each block's meta.json). After this PR, where the call toExists()as been removed, this case shouldn't be a real issue because we filter out the blocks that were marked for deletion before we start listing blocks in the bucket. Theblock.Delete()also guarantees that the deletion mark is the last file to be removed when deleting a block (andmeta.jsonis the first one), so this should reduce the race conditions we may have.Which issue(s) this PR fixes or relates to
Fixes #5056
Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX]