Logs deletion fixes #4625
Merged
Logs deletion fixes #4625
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
cyriltovena
reviewed
Nov 3, 2021
owen-d
reviewed
Nov 3, 2021
pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager.go
Outdated
Show resolved
Hide resolved
cyriltovena
approved these changes
Nov 3, 2021
… could span multiple tables
Co-authored-by: Cyril Tovena <cyril.tovena@gmail.com>
sandeepsukhani
force-pushed
the
logs-deletion-check-all-tables
branch
from
82e7ae5
to
ee3819e
Compare
|
Thanks for the review folks! I have taken care of the suggestions. |
| t.markerMetrics.tableProcessedTotal.WithLabelValues(tableName, tableActionNone).Inc() | ||
| return empty, markerWriter.Count(), nil | ||
| return empty, modified, nil |
There was a problem hiding this comment.
tiny nit: but I think it makes it slightly easier to read to hard-code this value to false, as it doesn't require the reader to invert the condition (which is already an inversion) that got them here,
| } | ||
| t.markerMetrics.tableProcessedTotal.WithLabelValues(tableName, tableActionModified).Inc() | ||
| return empty, markerWriter.Count(), nil | ||
| return empty, modified, nil |
There was a problem hiding this comment.
same here with hard-coding to true
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 / why we need it:
It includes the following two changes:
We check for whether the table would have chunks requested for deletion based on the delete request interval and the interval for which the table is expected to hold data or get queries, calculated using the table number. The problem here though is a chunk could overlap multiple tables so if a delete request is deleting the chunk for just one of the days, the index entry would stay untouched for the days for which the chunk has not been requested for deletion. To avoid this problem I am making compactor to go through all the index tables since it is hard to know how many tables a chunk could overlap. I had noticed chunks overlapping as long as 7 tables so it is safe to not skip any tables for now, until I can find a way to make this optimization work for delete requests too.
We upload the modified index from a table only when a chunk indexed in it has been marked for deletion. With some conditions, we sometimes just drop the index entry without marking chunk for deletion. This PR adds a check for it and uploads the index when it is modified even without marking any chunks for deletion.
Checklist
CHANGELOG.mdabout the changes.