Store/Receivers: Calculating chunk hashes on stores/receivers #5703

pedro-stanaka · 2022-09-19T13:33:47Z

I added CHANGELOG entry for this change.
Change is not relevant to the end user.

Related to #5682

Changes

Change multiple implementations of Series() in store to pre-calculate hashes for faster deduplication on querier:
- TSDB
- Prometheus
- Block storage

Verification

pkg/store/bucket.go

pkg/store/bucket_test.go

pkg/store/bucket.go

pkg/store/tsdb_test.go

bwplotka

Looks good! Just one nit - I would remove request field - I think we want to always offload hash to stores.

pkg/store/storepb/rpc.proto

bwplotka · 2022-09-20T14:05:37Z

pkg/store/storepb/rpc.proto

@@ -122,6 +122,9 @@ message SeriesRequest {
  // shard_info is used by the querier to request a specific
  // shard of blocks instead of entire blocks.
  ShardInfo shard_info = 13;
+
+  // Control whether the store should calculate the checksum/hash of chunks
+  bool calculate_chunk_checksums = 14;


Should we remove this and control that in the implementations (each server option for all requests). I don't see a need for request to ask for it explicitly 🤔

On the Thanos side I think we can always enable it. But Cortex also relies on the store gateway and the store API . Calculating hash on the cortex side is unnecessary for now

Please let me know what is best here. :)

I think it's fine to have it as a command-line argument instead of adding it to the request itself. That's what @bwplotka had in mind I believe.

Umm yeah, sounds like a flag on store gateway is better

If I got this right, we would need still to let them pass an option here: https://github.com/cortexproject/cortex/blob/d4fceb1172fff9ac332e525086d07f2ef182aefc/pkg/storegateway/bucket_stores.go#L484, right? Should I create a field on the bucket store to set wether or not to calculate hashes? Then, they could write a BucketStoreOption func that allows them to set this field. Let me know if this is okay.

Sorry for the late reply. For me I think it is fine to add it at blockSeries to control that option. Do you think it works?

I did that, I just thought that since that is a private function of the package they were not able to call it themselves.

Ah you are right sorry. So we need either:

an option at BucketStore to turn on/off chunk hashing

modify series request

Can you please recheck @yeya24 , I went with my idea to create a BucketStoreOption and enabled it by default on thanos side.

pkg/store/bucket.go

yeya24

LGTM

pkg/store/prometheus.go

yeya24 · 2022-09-25T22:01:01Z

pkg/store/storepb/rpc.proto

@@ -122,6 +122,9 @@ message SeriesRequest {
  // shard_info is used by the querier to request a specific
  // shard of blocks instead of entire blocks.
  ShardInfo shard_info = 13;
+
+  // Control whether the store should calculate the checksum/hash of chunks
+  bool calculate_chunk_checksums = 14;


If we don't need to change the protobuf file, then seems it is also okay to not add any flags.
Since Cortex only shares the store gateway with Thanos, so as long as the blockSeries method exposes a boolean param to control not calculate hash then it is fine. On the thanos side, we can make it always true.

pkg/store/storepb/testutil/series.go

yeya24

LGTM

douglascamata · 2022-10-14T15:44:34Z

@pedro-stanaka can you push an empty commit please to retrigger the CI? I think something happened to Github Actions and they didn't run after your last push.

Signed-off-by: Pedro Tanaka <[email protected]>

…ashes Signed-off-by: Pedro Tanaka <[email protected]>

Signed-off-by: Pedro Tanaka <[email protected]>

Signed-off-by: Pedro Tanaka <[email protected]> fixing flaky e2e tests Signed-off-by: Pedro Tanaka <[email protected]> fix integration test for prometheus Signed-off-by: Pedro Tanaka <[email protected]>

Signed-off-by: Pedro Tanaka <[email protected]> Final fix on error linting Signed-off-by: Pedro Tanaka <[email protected]>

Signed-off-by: Pedro Tanaka <[email protected]>

pedro-stanaka · 2022-10-14T16:29:36Z

@pedro-stanaka can you push an empty commit please to retrigger the CI? I think something happened to Github Actions and they didn't run after your last push.

Hey Douglas. Thanks for taking a look at this. I rebased and fixed some errors regarding new code from main, should be fine now.

Signed-off-by: Pedro Tanaka <[email protected]>

fpetkovski

This looks good. Should we merge it?

yeya24

LGTM

GiedriusS

Now we need to start using this here ;P

…-io#5703) * Adding hashes to raw chunks and populating it from bucket store Signed-off-by: Pedro Tanaka <[email protected]> * Adding flag on SeriesRequest to control whether to calculate or not hashes Signed-off-by: Pedro Tanaka <[email protected]> * Only calculate hash on series request for bucket when needed Signed-off-by: Pedro Tanaka <[email protected]> * implement e2e test for prometheus store Signed-off-by: Pedro Tanaka <[email protected]> fixing flaky e2e tests Signed-off-by: Pedro Tanaka <[email protected]> fix integration test for prometheus Signed-off-by: Pedro Tanaka <[email protected]> * calculating hash on TSDB Series() request as well Signed-off-by: Pedro Tanaka <[email protected]> Final fix on error linting Signed-off-by: Pedro Tanaka <[email protected]> * Using sync.Pool to avoid gc pressure Signed-off-by: Pedro Tanaka <[email protected]> * Fixing whitespace Signed-off-by: Pedro Tanaka <[email protected]> * Fixing nit on proto Signed-off-by: Pedro Tanaka <[email protected]> * Fixing hashpool Signed-off-by: Pedro Tanaka <[email protected]> * Using pool from callee instead of inside helper function Signed-off-by: Pedro Tanaka <[email protected]> * Reusing the hasher pool in other places Signed-off-by: Pedro Tanaka <[email protected]> * Calculating hashes by default Signed-off-by: Pedro Tanaka <[email protected]> * Adjusting e2e and integration tests Signed-off-by: Pedro Tanaka <[email protected]> * fixing linting and e2e test Signed-off-by: Pedro Tanaka <[email protected]> * Fixing new call format for tsdb.NewHead Signed-off-by: Pedro Tanaka <[email protected]> * Adding option to control whether to hash chunks on stores or not Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]>

…-io#5703) * Adding hashes to raw chunks and populating it from bucket store Signed-off-by: Pedro Tanaka <[email protected]> * Adding flag on SeriesRequest to control whether to calculate or not hashes Signed-off-by: Pedro Tanaka <[email protected]> * Only calculate hash on series request for bucket when needed Signed-off-by: Pedro Tanaka <[email protected]> * implement e2e test for prometheus store Signed-off-by: Pedro Tanaka <[email protected]> fixing flaky e2e tests Signed-off-by: Pedro Tanaka <[email protected]> fix integration test for prometheus Signed-off-by: Pedro Tanaka <[email protected]> * calculating hash on TSDB Series() request as well Signed-off-by: Pedro Tanaka <[email protected]> Final fix on error linting Signed-off-by: Pedro Tanaka <[email protected]> * Using sync.Pool to avoid gc pressure Signed-off-by: Pedro Tanaka <[email protected]> * Fixing whitespace Signed-off-by: Pedro Tanaka <[email protected]> * Fixing nit on proto Signed-off-by: Pedro Tanaka <[email protected]> * Fixing hashpool Signed-off-by: Pedro Tanaka <[email protected]> * Using pool from callee instead of inside helper function Signed-off-by: Pedro Tanaka <[email protected]> * Reusing the hasher pool in other places Signed-off-by: Pedro Tanaka <[email protected]> * Calculating hashes by default Signed-off-by: Pedro Tanaka <[email protected]> * Adjusting e2e and integration tests Signed-off-by: Pedro Tanaka <[email protected]> * fixing linting and e2e test Signed-off-by: Pedro Tanaka <[email protected]> * Fixing new call format for tsdb.NewHead Signed-off-by: Pedro Tanaka <[email protected]> * Adding option to control whether to hash chunks on stores or not Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Giedrius Statkevičius <[email protected]>

…-io#5703) * Adding hashes to raw chunks and populating it from bucket store Signed-off-by: Pedro Tanaka <[email protected]> * Adding flag on SeriesRequest to control whether to calculate or not hashes Signed-off-by: Pedro Tanaka <[email protected]> * Only calculate hash on series request for bucket when needed Signed-off-by: Pedro Tanaka <[email protected]> * implement e2e test for prometheus store Signed-off-by: Pedro Tanaka <[email protected]> fixing flaky e2e tests Signed-off-by: Pedro Tanaka <[email protected]> fix integration test for prometheus Signed-off-by: Pedro Tanaka <[email protected]> * calculating hash on TSDB Series() request as well Signed-off-by: Pedro Tanaka <[email protected]> Final fix on error linting Signed-off-by: Pedro Tanaka <[email protected]> * Using sync.Pool to avoid gc pressure Signed-off-by: Pedro Tanaka <[email protected]> * Fixing whitespace Signed-off-by: Pedro Tanaka <[email protected]> * Fixing nit on proto Signed-off-by: Pedro Tanaka <[email protected]> * Fixing hashpool Signed-off-by: Pedro Tanaka <[email protected]> * Using pool from callee instead of inside helper function Signed-off-by: Pedro Tanaka <[email protected]> * Reusing the hasher pool in other places Signed-off-by: Pedro Tanaka <[email protected]> * Calculating hashes by default Signed-off-by: Pedro Tanaka <[email protected]> * Adjusting e2e and integration tests Signed-off-by: Pedro Tanaka <[email protected]> * fixing linting and e2e test Signed-off-by: Pedro Tanaka <[email protected]> * Fixing new call format for tsdb.NewHead Signed-off-by: Pedro Tanaka <[email protected]> * Adding option to control whether to hash chunks on stores or not Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]> Signed-off-by: Pedro Tanaka <[email protected]>

pull-request-size bot added the size/L label Sep 19, 2022

sonatype-lift bot reviewed Sep 19, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

sonatype-lift bot reviewed Sep 19, 2022

View reviewed changes

pkg/store/bucket_test.go Outdated Show resolved Hide resolved

yeya24 reviewed Sep 19, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

yeya24 reviewed Sep 19, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from 459a21f to dcd040c Compare September 20, 2022 08:33

pull-request-size bot added size/XL and removed size/L labels Sep 20, 2022

sonatype-lift bot reviewed Sep 20, 2022

View reviewed changes

pkg/store/tsdb_test.go Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from a296efe to fa799bd Compare September 20, 2022 10:25

pedro-stanaka marked this pull request as ready for review September 20, 2022 10:35

bwplotka requested changes Sep 20, 2022

View reviewed changes

yeya24 reviewed Sep 20, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

yeya24 reviewed Sep 21, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from a4a2a51 to 2701f11 Compare September 22, 2022 06:56

yeya24 reviewed Sep 22, 2022

View reviewed changes

pkg/store/bucket.go Outdated Show resolved Hide resolved

yeya24 previously approved these changes Sep 23, 2022

View reviewed changes

yeya24 reviewed Sep 25, 2022

View reviewed changes

pedro-stanaka dismissed yeya24’s stale review via c4eb864 September 26, 2022 10:16

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from b82dbed to c4eb864 Compare September 26, 2022 10:16

pull-request-size bot added size/L and removed size/XL labels Sep 26, 2022

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from 0d19ff2 to 94be3bf Compare September 26, 2022 12:39

sonatype-lift bot reviewed Sep 26, 2022

View reviewed changes

pkg/store/storepb/testutil/series.go Outdated Show resolved Hide resolved

yeya24 previously approved these changes Sep 26, 2022

View reviewed changes

pedro-stanaka dismissed yeya24’s stale review via 83ee0ab October 5, 2022 16:03

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from 43502e8 to 83ee0ab Compare October 5, 2022 16:03

pedro-stanaka added 2 commits October 14, 2022 18:11

Adding hashes to raw chunks and populating it from bucket store

2445858

Signed-off-by: Pedro Tanaka <[email protected]>

Adding flag on SeriesRequest to control whether to calculate or not h…

44d21de

…ashes Signed-off-by: Pedro Tanaka <[email protected]>

pedro-stanaka and others added 12 commits October 14, 2022 18:11

Only calculate hash on series request for bucket when needed

e0afef2

Signed-off-by: Pedro Tanaka <[email protected]>

implement e2e test for prometheus store

b5f5b2f

Signed-off-by: Pedro Tanaka <[email protected]> fixing flaky e2e tests Signed-off-by: Pedro Tanaka <[email protected]> fix integration test for prometheus Signed-off-by: Pedro Tanaka <[email protected]>

calculating hash on TSDB Series() request as well

2f38f44

Signed-off-by: Pedro Tanaka <[email protected]> Final fix on error linting Signed-off-by: Pedro Tanaka <[email protected]>

Using sync.Pool to avoid gc pressure

3180870

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing whitespace

abeaa0c

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing nit on proto

55cae36

Signed-off-by: Pedro Tanaka <[email protected]>

Fixing hashpool

3631333

Signed-off-by: Pedro Tanaka <[email protected]>

Using pool from callee instead of inside helper function

566c423

Signed-off-by: Pedro Tanaka <[email protected]>

Reusing the hasher pool in other places

0a3f651

Signed-off-by: Pedro Tanaka <[email protected]>

Calculating hashes by default

d6ca5e9

Signed-off-by: Pedro Tanaka <[email protected]>

Adjusting e2e and integration tests

edd1894

Signed-off-by: Pedro Tanaka <[email protected]>

fixing linting and e2e test

2055036

Signed-off-by: Pedro Tanaka <[email protected]>

pedro-stanaka force-pushed the feat/hash-chunks-leaves branch from 83ee0ab to 2055036 Compare October 14, 2022 16:11

Fixing new call format for tsdb.NewHead

f2ccbdc

Signed-off-by: Pedro Tanaka <[email protected]>

Adding option to control whether to hash chunks on stores or not

2680da6

Signed-off-by: Pedro Tanaka <[email protected]>

fpetkovski approved these changes Oct 25, 2022

View reviewed changes

yeya24 approved these changes Oct 25, 2022

View reviewed changes

GiedriusS merged commit 2d0407f into thanos-io:main Oct 25, 2022

GiedriusS reviewed Oct 25, 2022

View reviewed changes

pedro-stanaka mentioned this pull request Nov 10, 2022

proxy: pre-calculate hash at leaf nodes #5682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store/Receivers: Calculating chunk hashes on stores/receivers #5703

Store/Receivers: Calculating chunk hashes on stores/receivers #5703

pedro-stanaka commented Sep 19, 2022 •

edited

Loading

bwplotka left a comment

bwplotka Sep 20, 2022

yeya24 Sep 20, 2022 •

edited

Loading

pedro-stanaka Sep 21, 2022

fpetkovski Sep 22, 2022

yeya24 Sep 23, 2022

pedro-stanaka Sep 26, 2022

yeya24 Oct 5, 2022

pedro-stanaka Oct 5, 2022

yeya24 Oct 14, 2022

pedro-stanaka Oct 17, 2022

yeya24 left a comment

yeya24 Sep 25, 2022

yeya24 left a comment

douglascamata commented Oct 14, 2022

pedro-stanaka commented Oct 14, 2022

fpetkovski left a comment

yeya24 left a comment

GiedriusS left a comment

Store/Receivers: Calculating chunk hashes on stores/receivers #5703

Store/Receivers: Calculating chunk hashes on stores/receivers #5703

Conversation

pedro-stanaka commented Sep 19, 2022 • edited Loading

Changes

Verification

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 Sep 20, 2022 • edited Loading

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

yeya24 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

douglascamata commented Oct 14, 2022

pedro-stanaka commented Oct 14, 2022

fpetkovski left a comment

Choose a reason for hiding this comment

yeya24 left a comment

Choose a reason for hiding this comment

GiedriusS left a comment

Choose a reason for hiding this comment

pedro-stanaka commented Sep 19, 2022 •

edited

Loading

yeya24 Sep 20, 2022 •

edited

Loading