-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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 a DeletionMode config variable #5481
Add a DeletionMode config variable #5481
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.
Thanks for making a start on this. I think it goes into the right direction, but I think for the time being we would still need to have delete functionality for the case when someone using Loki relies on it.
So my suggestion would be: Let use the new API endpoint name (and ideally the new promql
field), but still allow deletion of "selector-only" logql requests. Wdyt?
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
85e8989
to
28b5c89
Compare
Agreed. To expand a bit: |
We can, but this is an API marked as experimental so I would argue it's fine to change them. We're also changing the parameters for the PUT/POST. |
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.
I think this looks awesome
0fcd262
to
578cf50
Compare
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.
That's coming together really well. LGTM
A couple of nits in the comments.
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager_test.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_requests_store.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_requests_store.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_requests_store.go
Outdated
Show resolved
Hide resolved
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 incorporating the review. LGTM
I would like you to think about the v1
name again for the default/legacy, I think we should be more descritive v1
doesn't really mean anything.
521ad45
to
1ab48f7
Compare
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.
Added some comments but other than that it looks good to me.
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
8ba392d
to
b0b0196
Compare
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.
I think we should also update both the HTTP API page and the more specific docs. But this can also happen outside of this PR.
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Co-authored-by: Christian Simon <simon@swine.de>
…manager_test.go Co-authored-by: Christian Simon <simon@swine.de>
…store.go Co-authored-by: Christian Simon <simon@swine.de>
…store.go Co-authored-by: Christian Simon <simon@swine.de>
Co-authored-by: Christian Simon <simon@swine.de>
…store.go Co-authored-by: Christian Simon <simon@swine.de>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
3cd4522
to
85a32ea
Compare
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.
Just some minor nits. Thanks for taking care of all the feedback and nice work!
pkg/storage/stores/shipper/compactor/deletion/delete_request.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>
Signed-off-by: Michel Hollands <michel.hollands@grafana.com>
What this PR does / why we need it:
A new config variable called DeletionMode with a delete mode has been added as well. It's separate from the existing retention setting. It has a default value which uses the existing delete functionality This config variable will be used later on when the v2 deletion functionality is added.
Rename internal fields for future expansion of the delete requests.
Validate the delete requests.
Special notes for your reviewer:
Checklist
CHANGELOG.md
about the changes.