Add a DeletionMode config variable #5481

MichelHollands · 2022-02-25T16:31:46Z

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

Documentation updated
Tests updated
Add an entry in the CHANGELOG.md about the changes.

simonswine

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

pkg/loki/modules.go

pkg/storage/stores/shipper/compactor/compactor.go

pkg/storage/stores/shipper/compactor/deletion/delete_request.go

dannykopping · 2022-03-02T14:50:55Z

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.

Agreed. To expand a bit:
We should also be careful about changing API interfaces - can we maintain both the new and the old endpoints? We can mark the old ones as deprecated and remove them with the next major release.

MichelHollands · 2022-03-02T17:32:15Z

Agreed. To expand a bit: We should also be careful about changing API interfaces - can we maintain both the new and the old endpoints? We can mark the old ones as deprecated and remove them with the next major release.

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.

MasslessParticle

I think this looks awesome

simonswine

That's coming together really well. LGTM

A couple of nits in the comments.

pkg/storage/stores/shipper/compactor/deletion/delete_request.go

pkg/storage/stores/shipper/compactor/deletion/delete_requests_manager_test.go

pkg/storage/stores/shipper/compactor/deletion/delete_requests_store.go

pkg/storage/stores/shipper/compactor/compactor.go

CHANGELOG.md

pkg/storage/stores/shipper/compactor/compactor.go

simonswine

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.

pkg/storage/stores/shipper/compactor/deletion/mode.go

sandeepsukhani

Added some comments but other than that it looks good to me.

pkg/storage/stores/shipper/compactor/compactor.go

pkg/storage/stores/shipper/compactor/deletion/delete_request.go

simonswine

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>

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>

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>

sandeepsukhani

Just some minor nits. Thanks for taking care of all the feedback and nice work!

pkg/storage/stores/shipper/compactor/compactor.go

pkg/storage/stores/shipper/compactor/deletion/delete_request.go

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

pull-request-size bot added the size/XL label Feb 25, 2022

simonswine reviewed Feb 28, 2022

View reviewed changes

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Outdated Show resolved Hide resolved

pkg/loki/modules.go Show resolved Hide resolved

pkg/storage/stores/shipper/compactor/compactor.go Outdated Show resolved Hide resolved

pull-request-size bot added size/L size/XL and removed size/XL size/L labels Mar 1, 2022

simonswine reviewed Mar 1, 2022

View reviewed changes

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Outdated Show resolved Hide resolved

MichelHollands changed the title ~~Add delete api to compactor~~ Update the delete api so it looks more like the other API endpoints Mar 2, 2022

MichelHollands force-pushed the add_delete_v2_api_to_compactor branch from 85e8989 to 28b5c89 Compare March 2, 2022 14:23

MichelHollands marked this pull request as ready for review March 2, 2022 14:48

MichelHollands requested review from KMiller-Grafana and a team as code owners March 2, 2022 14:48

MasslessParticle approved these changes Mar 2, 2022

View reviewed changes

MasslessParticle mentioned this pull request Mar 4, 2022

Query Filtering -- Querier #5482

Merged

MichelHollands force-pushed the add_delete_v2_api_to_compactor branch 2 times, most recently from 0fcd262 to 578cf50 Compare March 15, 2022 14:39

MichelHollands closed this Mar 15, 2022

MichelHollands reopened this Mar 15, 2022

MichelHollands changed the title ~~Update the delete api so it looks more like the other API endpoints~~ Add a DeletionMode config variable Mar 15, 2022

simonswine approved these changes Mar 15, 2022

View reviewed changes

MasslessParticle mentioned this pull request Mar 15, 2022

Query filtering in the ingester and storage #5629

Merged

3 tasks

simonswine approved these changes Mar 17, 2022

View reviewed changes

pkg/storage/stores/shipper/compactor/deletion/mode.go Outdated Show resolved Hide resolved

MichelHollands force-pushed the add_delete_v2_api_to_compactor branch from 521ad45 to 1ab48f7 Compare March 18, 2022 08:56

sandeepsukhani reviewed Mar 24, 2022

View reviewed changes

pkg/storage/stores/shipper/compactor/compactor.go Outdated Show resolved Hide resolved

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

MichelHollands force-pushed the add_delete_v2_api_to_compactor branch from 8ba392d to b0b0196 Compare April 4, 2022 07:58

simonswine approved these changes Apr 4, 2022

View reviewed changes

MichelHollands and others added 25 commits April 6, 2022 09:52

Update changelog

dc4ab08

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Revert only the API surface area while keeping everything else

b00482c

Use moved code in syntax package

38d819f

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Remove duplicte import

3ae0aea

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Use renamed field in tests

b6b4734

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Remove duplicates and empty lines in changelog

beddf81

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Update changelog description

e3dbe38

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Update pkg/storage/stores/shipper/compactor/deletion/delete_request.go

1ba285a

Co-authored-by: Christian Simon <simon@swine.de>

Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_…

8152916

…manager_test.go Co-authored-by: Christian Simon <simon@swine.de>

Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_…

eb46a59

…store.go Co-authored-by: Christian Simon <simon@swine.de>

Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_…

197e08b

…store.go Co-authored-by: Christian Simon <simon@swine.de>

Update CHANGELOG.md

ff27a1b

Co-authored-by: Christian Simon <simon@swine.de>

Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_…

d249f18

…store.go Co-authored-by: Christian Simon <simon@swine.de>

Make DeletionMode struct member non public

0b35252

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Revert change to docs re cancellation

0b49512

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Use same variable names

1db21ca

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Add parameter validation to changelog

92aa117

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Rename v1 to WholeStreamDeletion

a574d62

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Fix default value of deletion mode config setting

8098e1b

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

reimplement new api

86a2a53

Add delete request handler when delete mode is set

16e9ab2

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Remove unused variable

9bec8a1

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Add comment to change the code when other deletion modes are available

a10466b

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

create expirationChecker if deletionMode is set

f9addc8

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

Address review comments

85a32ea

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

MichelHollands force-pushed the add_delete_v2_api_to_compactor branch from 3cd4522 to 85a32ea Compare April 6, 2022 09:00

sandeepsukhani approved these changes Apr 6, 2022

View reviewed changes

pkg/storage/stores/shipper/compactor/compactor.go Outdated Show resolved Hide resolved

pkg/storage/stores/shipper/compactor/deletion/delete_request.go Outdated Show resolved Hide resolved

MichelHollands and others added 2 commits April 6, 2022 10:31

Update pkg/storage/stores/shipper/compactor/compactor.go

a12b157

Co-authored-by: Sandeep Sukhani <sandeep.d.sukhani@gmail.com>

Rename AddQuery to SetQuery

9d6dc0d

Signed-off-by: Michel Hollands <michel.hollands@grafana.com>

sandeepsukhani merged commit b865b81 into grafana:main Apr 6, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a DeletionMode config variable #5481

Add a DeletionMode config variable #5481

MichelHollands commented Feb 25, 2022 •

edited

Loading

simonswine left a comment

dannykopping commented Mar 2, 2022

MichelHollands commented Mar 2, 2022

MasslessParticle left a comment

simonswine left a comment

simonswine left a comment

sandeepsukhani left a comment

simonswine left a comment

sandeepsukhani left a comment

Add a DeletionMode config variable #5481

Add a DeletionMode config variable #5481

Conversation

MichelHollands commented Feb 25, 2022 • edited Loading

simonswine left a comment

Choose a reason for hiding this comment

dannykopping commented Mar 2, 2022

MichelHollands commented Mar 2, 2022

MasslessParticle left a comment

Choose a reason for hiding this comment

simonswine left a comment

Choose a reason for hiding this comment

simonswine left a comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

simonswine left a comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

MichelHollands commented Feb 25, 2022 •

edited

Loading