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.

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

Block deletion endpoints #1

Open
wants to merge 28 commits into
base: master
Choose a base branch
from
Open

Conversation

ilangofman
Copy link
Owner

@ilangofman ilangofman commented Jul 13, 2021

What this PR does:

This PR is part of the proposed addition of the time series deletion API for block storage.

The current plan is to split the feature into 4 smaller PR's:

  • API endpoints and creation of tombstones (This PR)
  • Permanent deletion processing
  • Query time filtering
  • Cache invalidation

Which issue(s) this PR fixes:
Fixes #

Checklist

  • Tests updated
  • Documentation added
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

Sorry, something went wrong.

pkg/api/api.go Outdated
a.RegisterRoute("/purger/delete_tenant_status", http.HandlerFunc(api.DeleteTenantStatus), true, "GET")
func (a *API) RegisterBlocksPurgerAPI(blocksPurger *purger.BlocksPurgerAPI) {

a.RegisterRoute(path.Join(a.cfg.PrometheusHTTPPrefix, "/api/v1/admin/tsdb/delete_series"), http.HandlerFunc(blocksPurger.V2AddDeleteRequestHandler), true, "PUT", "POST")

Choose a reason for hiding this comment

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

This isn't really V2 right? Should there be V2 in the name?

Copy link
Owner Author

Choose a reason for hiding this comment

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

These functions exist for the chunks purger. I thought adding the V2 like in the ingester functions would make things more clear. They are in the blocksPurger so that might be enough of information to tell them apart. I will remove it.

pkg/api/api.go Outdated Show resolved Hide resolved
pkg/chunk/purger/blocks_purger.go Show resolved Hide resolved
pkg/chunk/purger/blocks_purger.go Outdated Show resolved Hide resolved
pkg/chunk/purger/blocks_purger.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/tombstones.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/tombstones.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/tombstones.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/tombstones.go Outdated Show resolved Hide resolved
pkg/storage/tsdb/tombstones.go Show resolved Hide resolved
@ilangofman ilangofman force-pushed the block_deletion_endpoints branch from 0166c79 to 220a47e Compare July 14, 2021 22:17
Signed-off-by: ilangofman <[email protected]>
pkg/chunk/purger/blocks_purger.go Outdated Show resolved Hide resolved
// add all the tombstones to a map and check for duplicates,
// if a key exists with the same request ID (but two different states)
tombstoneMap := make(map[string]*Tombstone)
err := userBucket.Iter(ctx, "tombstones/", func(s string) error {
Copy link

@harry671003 harry671003 Jul 14, 2021

Choose a reason for hiding this comment

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

I think this too can be simplified as follows:

  • Initialize the tombstone map
  • Sort all the tombstones in the bucket by state. https://gobyexample.com/sorting-by-functions
  • Iterate over the sorted tombstones. set tombstonesMap[id] = tombstoneFile
  • What's left in the map would be the most recent state.

Copy link
Owner Author

Choose a reason for hiding this comment

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

As per your other comment, I removed the removeDuplicateTombstone call from the get functions and add it during the cleanup in the compactor. This should simplify it a bit.

I think with the suggestion you made, it might be less efficient if I interpreted it correctly:

  • Iterate over the tombstones in the bucket and add them all to a slice.
  • Sort the slice
  • Iterate over the slice and add it to the map
  • iterate over the map and add it to a new slice
  • return the slice

This will add another pass over the slice and a sort operation.

Let me know if I misinterpreted your suggestion.

// If there are multiple tombstones with the same request id but different state, should delete the files with the lower state
for _, ts := range found[0 : len(found)-1] {
level.Info(userLogger).Log("msg", "Found extra tombstone file with outdated state. Will delete it.", "requestID", ts.RequestID, "state", ts.State)
if err := DeleteTombstoneFile(ctx, bkt, cfgProvider, ts); err != nil {
Copy link

@harry671003 harry671003 Jul 14, 2021

Choose a reason for hiding this comment

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

I don't like the fact that we are deleting tombstones during a GET operation. Can't we just perform clean up in the compactor after deletion is complete?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah I think that makes more sense. I removed the deletion operations in the get functions.

Copy link

@harry671003 harry671003 left a comment

Choose a reason for hiding this comment

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

LGTM

ilangofman and others added 12 commits July 15, 2021 16:35
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>
ilangofman and others added 9 commits August 16, 2021 10:44
Signed-off-by: ilangofman <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>
Signed-off-by: ilangofman <[email protected]>

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
reverse change of combining blocks purger and tenant deletion
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants