Fix delete updates #6194

MasslessParticle · 2022-05-18T23:21:06Z

Previously the querier got deletes from the index gateway. The delete_requests table was never updated because the component assumes that filenames never change once written (which is how the index files work). Rather than changing the index gateway's assumption about indicies or carving out an exception, this PR makes the querier get delete requests from the delete API itself. That way, there is only one source of truth for deletes and everything just pulls from there.

This PR also changes the location of the compactor_address configuration. Rather than have it on each component that needs it, the address is now part of the compactor config where any component can just grab it.

grafanabot · 2022-05-18T23:25:50Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.2%

sandeepsukhani · 2022-05-19T06:23:00Z

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

@@ -87,6 +88,7 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
 	f.DurationVar(&cfg.DeleteRequestCancelPeriod, "boltdb.shipper.compactor.delete-request-cancel-period", 24*time.Hour, "Allow cancellation of delete request until duration after they are created. Data would be deleted only after delete requests have been older than this duration. Ideally this should be set to at least 24h.")
 	f.IntVar(&cfg.MaxCompactionParallelism, "boltdb.shipper.compactor.max-compaction-parallelism", 1, "Maximum number of tables to compact in parallel. While increasing this value, please make sure compactor has enough disk space allocated to be able to store and compact as many tables.")
 	f.StringVar(&cfg.DeletionMode, "boltdb.shipper.compactor.deletion-mode", "whole-stream-deletion", fmt.Sprintf("(Experimental) Deletion mode. Can be one of %v", strings.Join(deletion.AllModes(), "|")))
+	f.StringVar(&cfg.Address, "boltdb.shipper.compactor.address", "", "host and port where the compactor API is listening")


Adding this config to the compactor looks out of place to me. New users already get confused by our configs, so it is better to duplicate it a little than add to the confusion. It could be just me. What do you think?

I went back and forth on this. If we leave it off the compactor config, it becomes a required property on:

frontend

querier

ruler

Do you think that's clearer for the user than having the compactor know it's own address?

What do you think about making it part of storage config? It is anyways something related to storage.

is there also a case to be made for the common config, since it's common across multiple components?

Common Config says // Values defined under this common configuration are supersede if a more specific value is defined.

Does that imply that there's also some more specific override elsewhere? If not, it would be a fine place for this to go.

I don't think that would apply in this case, if we moved it to the common config so that was the only place it's defined, then there would be no superseding. What that comment is meant to convey is that after things from the common config (like storage or ring configs) are applied to the many places those are used, the config.yaml is read again, so if there is say, a specific ring config for the scheduler, that will supersede whatever was put in common.

yeah, I thought about using common config but due to that comment I thought it was not the right place, same as Travis. If we can use it for this use case then it would be a better option.

sandeepsukhani · 2022-05-19T06:35:37Z

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

+	}
+}
+
+func NewDeleteRequestsClient(addr string, c doer, opts ...DeleteRequestsStoreOption) (DeleteRequestsClient, error) {


Instead of taking an HTTP client as a parameter, what do you think about accepting a config?

I'd prefer to take the client, if it's not too disruptive. I really like this pattern because it makes if very easy to unit test without worrying about the network: pkg/storage/stores/shipper/compactor/deletion/delete_requests_client_test.go

sandeepsukhani

Overall the changes look great to me. I just added 2 comments. Irrelevant to this PR, I also think we should add an in-memory cache for CacheGenClient same as DeleteRequestsClient.

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

…client.go Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>

MasslessParticle · 2022-05-23T15:01:12Z

Caching gennumbers should be covered by the gennumber_loader here

grafanabot · 2022-05-23T15:12:27Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.2%

sandeepsukhani

I did another pass, just added some minor suggestions.

pkg/loki/modules.go

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

grafanabot · 2022-05-25T17:32:36Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

grafanabot · 2022-05-25T20:26:03Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
-        distributor	-0.3%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

sandeepsukhani

Left very minor suggestions. Let us see if we want to move compactor_address to common config.

production/ksonnet/loki/boltdb_shipper.libsonnet

docs/sources/configuration/_index.md

grafanabot · 2022-05-26T18:15:46Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.9%

grafanabot · 2022-05-26T18:16:08Z

./tools/diff_coverage.sh ../loki-main/test_results.txt test_results.txt ingester,distributor,querier,querier/queryrange,iter,storage,chunkenc,logql,loki

Change in test coverage per package. Green indicates 0 or positive change, red indicates that test coverage for a package fell.

+           ingester	0%
+        distributor	0%
+            querier	0%
+ querier/queryrange	0%
+               iter	0%
+            storage	0%
+           chunkenc	0%
+              logql	0%
-               loki	-0.3%

sandeepsukhani

LGTM

* Fix delete updates * Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_client.go Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com> * Add compactor address to the storage config * review comments * Review feedback Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com> (cherry picked from commit 9b2786b)

* Fix delete updates * Update pkg/storage/stores/shipper/compactor/deletion/delete_requests_client.go Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com> * Add compactor address to the storage config * review comments * Review feedback Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com> (cherry picked from commit 9b2786b) Co-authored-by: Travis Patterson <travis.patterson@grafana.com>

Fix delete updates

51d1535

MasslessParticle requested review from KMiller-Grafana and a team as code owners May 18, 2022 23:21

pull-request-size bot added the size/L label May 18, 2022

github-actions bot added the area/docs label May 18, 2022

MasslessParticle requested review from MichelHollands, simonswine and sandeepsukhani May 18, 2022 23:21

sandeepsukhani reviewed May 19, 2022

View reviewed changes

MichelHollands reviewed May 19, 2022

View reviewed changes

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

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

40e9df3

…client.go Co-authored-by: Michel Hollands <42814411+MichelHollands@users.noreply.github.com>

Merge branch 'main' into tpatterson/fix-delete-updates

53a52fb

sandeepsukhani reviewed May 24, 2022

View reviewed changes

pkg/loki/modules.go Show resolved Hide resolved

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

MasslessParticle added 2 commits May 25, 2022 09:43

Merge branch 'main' into tpatterson/fix-delete-updates

706f3d6

Add compactor address to the storage config

8b73fbd

review comments

c6c27d2

sandeepsukhani reviewed May 26, 2022

View reviewed changes

production/ksonnet/loki/boltdb_shipper.libsonnet Outdated Show resolved Hide resolved

docs/sources/configuration/_index.md Outdated Show resolved Hide resolved

MasslessParticle added 2 commits May 26, 2022 12:10

Review feedback

41cffbc

Merge branch 'main' into tpatterson/fix-delete-updates

942a64e

sandeepsukhani approved these changes May 30, 2022

View reviewed changes

sandeepsukhani merged commit 9b2786b into grafana:main May 30, 2022

MasslessParticle added the backport k100 label Jun 2, 2022

grafanabot mentioned this pull request Jun 2, 2022

[k100] Fix delete updates #6291

Merged

This was referenced Jun 28, 2022

dannykopping/remove cache stats dannykopping/loki#13

Closed

dannykopping/remove cache stats dannykopping/loki#14

Draft

osg-grafana added type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories and removed area/docs labels Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix delete updates #6194

Fix delete updates #6194

MasslessParticle commented May 18, 2022

grafanabot commented May 18, 2022

sandeepsukhani May 19, 2022

MasslessParticle May 23, 2022

sandeepsukhani May 24, 2022

trevorwhitney May 25, 2022

MasslessParticle May 25, 2022

trevorwhitney May 25, 2022

sandeepsukhani May 26, 2022

sandeepsukhani May 19, 2022

MasslessParticle May 23, 2022

sandeepsukhani left a comment

MasslessParticle commented May 23, 2022

grafanabot commented May 23, 2022

sandeepsukhani left a comment

grafanabot commented May 25, 2022

grafanabot commented May 25, 2022

sandeepsukhani left a comment

grafanabot commented May 26, 2022

grafanabot commented May 26, 2022

sandeepsukhani left a comment

Fix delete updates #6194

Fix delete updates #6194

Conversation

MasslessParticle commented May 18, 2022

grafanabot commented May 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandeepsukhani left a comment

Choose a reason for hiding this comment

MasslessParticle commented May 23, 2022

grafanabot commented May 23, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

grafanabot commented May 25, 2022

grafanabot commented May 25, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment

grafanabot commented May 26, 2022

grafanabot commented May 26, 2022

sandeepsukhani left a comment

Choose a reason for hiding this comment