bloom blocks downloading queue #11201

vlad-diachenko · 2023-11-10T14:17:34Z

implemented bloom blocks downloading queue to control the concurrency of downloading the blocks from the storage

Signed-off-by: Vladyslav Diachenko <[email protected]>

pkg/storage/stores/shipper/bloomshipper/block_downloader.go

pkg/storage/stores/shipper/bloomshipper/shipper.go

owen-d

took a brief look, letting @chaudum do the lions share of review on this one

pkg/storage/stores/shipper/bloomshipper/block_downloader.go

# Conflicts: # pkg/validation/limits.go

Signed-off-by: Vladyslav Diachenko <[email protected]>

github-actions · 2023-11-21T16:48:40Z

Trivy scan found the following vulnerabilities:

HIGH openssl: Incorrect cipher key and IV length processing in libcrypto3 v3.1.3-r0. Fixed in v3.1.4-r0
HIGH openssl: Incorrect cipher key and IV length processing in libssl3 v3.1.3-r0. Fixed in v3.1.4-r0

Signed-off-by: Vladyslav Diachenko <[email protected]>

…ng additional goroutine Signed-off-by: Vladyslav Diachenko <[email protected]>

pkg/storage/stores/shipper/bloomshipper/block_downloader.go

chaudum · 2023-11-22T13:37:40Z

pkg/storage/stores/shipper/bloomshipper/block_downloader.go

+	// we need to have errCh with size that can keep max count of errors to prevent the case when
+	// the queue worker reported the error to this channel before the current goroutine
+	// and this goroutine will go to the deadlock because it won't be able to report an error
+	// because nothing reads this channel at this moment.
+	errCh := make(chan error, len(references))


Not sure if I understand that correctly. On line 168 you return after sending the first and only error to the errCh. Could you just make buffered channel of size 1?

this channel is also passed to the queue and queue workers will report errors to this channel also...

pkg/storage/stores/shipper/bloomshipper/config/config.go

Signed-off-by: Vladyslav Diachenko <[email protected]>

poyzannur · 2023-11-23T11:48:21Z

pkg/storage/stores/shipper/bloomshipper/client.go

+		return Block{}, fmt.Errorf("error while period lookup: %w", err)
+	}
+	objectClient := b.periodicObjectClients[period]
+	readCloser, _, err := objectClient.GetObject(ctx, createBlockObjectKey(reference.Ref))


I know you'll see this with the next rebase, but now result of this get call is a compressed tar archive that needs to be downloaded and decompressed, which needs to be handled

it's already handled in

loki/pkg/storage/stores/shipper/bloomshipper/block_downloader.go

Line 134 in d96c939

directory, err := d.extractBlock(&block, time.Now())

poyzannur · 2023-11-23T12:16:35Z

pkg/storage/stores/shipper/bloomshipper/client.go

-	}()
-	return blocksChannel, errChannel
+// GetBlock downloads the blocks from objectStorage and returns the downloaded block
+func (b *BloomClient) GetBlock(ctx context.Context, reference BlockRef) (Block, error) {


asking to understand, is there no use case fetching multiple blocks at once? i can see underlying call is the same to object store, i'm asking from api design perspective.

we will download many blocks in parallel using the downloading queue. I see that in the compactor it might be necessary to download many blocks in parallel also, but I believe we can spin up the goroutines in compactor's code to download many blocks in parallel. wdyt?

# Conflicts: # pkg/validation/limits.go

Signed-off-by: Vladyslav Diachenko <[email protected]>

chaudum

Left a few comments, but nothing that would block merging.

chaudum · 2023-11-24T10:21:53Z

pkg/bloomgateway/bloomgateway_test.go

+	//TODO implement me
+	panic("implement me")


Suggested change

//TODO implement me

panic("implement me")

return 0

chaudum · 2023-11-24T10:22:15Z

pkg/bloomgateway/bloomgateway_test.go

+	//TODO implement me
+	panic("implement me")


Suggested change

//TODO implement me

panic("implement me")

return true

chaudum · 2023-11-24T10:22:25Z

pkg/bloomgateway/bloomgateway_test.go

+	//TODO implement me
+	panic("implement me")


Suggested change

//TODO implement me

panic("implement me")

return 1

chaudum · 2023-11-24T10:24:45Z

pkg/storage/stores/shipper/bloomshipper/block_downloader.go

+}
+
+func (d *blockDownloader) stop() {
+	_ = services.StopManagerAndAwaitStopped(d.ctx, d.manager)


Maybe we should log the error?

chaudum · 2023-11-24T10:26:20Z

pkg/storage/stores/shipper/bloomshipper/config/config.go

+}
+
+func (cfg *DownloadingQueueConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
+	f.IntVar(&cfg.WorkersCount, prefix+"workers-count", 100, "The count of parallel workers that download Bloom Blocks.")


I think we should be more conservative regarding the default value. 10 should be more than sufficient?

chaudum · 2023-11-24T10:26:37Z

pkg/storage/stores/shipper/bloomshipper/config/config.go

+
+func (cfg *DownloadingQueueConfig) RegisterFlagsWithPrefix(prefix string, f *flag.FlagSet) {
+	f.IntVar(&cfg.WorkersCount, prefix+"workers-count", 100, "The count of parallel workers that download Bloom Blocks.")
+	f.IntVar(&cfg.MaxTasksEnqueuedPerTenant, prefix+"max_tasks_enqueued_per_tenant", 10_000, "Maximum number of task in queue per tenant per bloom-gateway. Enqueuing the tasks above this limit will fail an error.")


implemented bloom blocks downloading queue to control the concurrency of downloading the blocks from the storage Signed-off-by: Vladyslav Diachenko <[email protected]>

implemented bloom blocks downloading queue

73895e2

Signed-off-by: Vladyslav Diachenko <[email protected]>

vlad-diachenko requested a review from a team as a code owner November 10, 2023 14:17

pull-request-size bot added the size/XL label Nov 10, 2023

Merge branch 'main' into vlad.diachenko/bloom-blocks-downloading-queue

8c229b8

vlad-diachenko enabled auto-merge (squash) November 10, 2023 14:21

chaudum reviewed Nov 13, 2023

View reviewed changes

owen-d reviewed Nov 15, 2023

View reviewed changes

pkg/storage/stores/shipper/bloomshipper/block_downloader.go Show resolved Hide resolved

vlad-diachenko added 2 commits November 21, 2023 18:36

Merge branch 'main' into vlad.diachenko/bloom-blocks-downloading-queue

61ab0f0

# Conflicts: # pkg/validation/limits.go

reformatted the code

7d2f0bf

Signed-off-by: Vladyslav Diachenko <[email protected]>

vlad-diachenko added 3 commits November 22, 2023 12:13

added context with cancellation to properly stop the workers

6b05ec5

Signed-off-by: Vladyslav Diachenko <[email protected]>

use service manager to manage queue and activeUsersCleanup lifecycles

f65974f

Signed-off-by: Vladyslav Diachenko <[email protected]>

moved block querier creation logic to the queue worker to avoid runni…

3c4937b

…ng additional goroutine Signed-off-by: Vladyslav Diachenko <[email protected]>

chaudum reviewed Nov 22, 2023

View reviewed changes

addressed comments on PR

d96c939

Signed-off-by: Vladyslav Diachenko <[email protected]>

poyzannur reviewed Nov 23, 2023

View reviewed changes

vlad-diachenko added 4 commits November 24, 2023 09:43

Merge branch 'main' into vlad.diachenko/bloom-blocks-downloading-queue

67db11d

# Conflicts: # pkg/validation/limits.go

passed the limits to the blooms shipper

a58a1db

Signed-off-by: Vladyslav Diachenko <[email protected]>

fixed the issues reported by the linter

c31ab4d

Signed-off-by: Vladyslav Diachenko <[email protected]>

fixed docs and imports

c9ee29d

Signed-off-by: Vladyslav Diachenko <[email protected]>

github-actions bot added the type/docs Issues related to technical documentation; the Docs Squad uses this label across many repositories label Nov 24, 2023

vlad-diachenko and others added 2 commits November 24, 2023 10:22

fixed imports format

ba28e04

Signed-off-by: Vladyslav Diachenko <[email protected]>

Merge branch 'main' into vlad.diachenko/bloom-blocks-downloading-queue

75f3184

vlad-diachenko requested review from chaudum, poyzannur and salvacorts November 24, 2023 10:05

chaudum approved these changes Nov 24, 2023

View reviewed changes

vlad-diachenko merged commit 75cfe59 into main Nov 24, 2023
6 checks passed

vlad-diachenko deleted the vlad.diachenko/bloom-blocks-downloading-queue branch November 24, 2023 10:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bloom blocks downloading queue #11201

bloom blocks downloading queue #11201

vlad-diachenko commented Nov 10, 2023

owen-d left a comment

github-actions bot commented Nov 21, 2023 •

edited

Loading

chaudum Nov 22, 2023

vlad-diachenko Nov 22, 2023

poyzannur Nov 23, 2023

vlad-diachenko Nov 23, 2023

poyzannur Nov 23, 2023

vlad-diachenko Nov 23, 2023

chaudum left a comment

chaudum Nov 24, 2023

chaudum Nov 24, 2023

chaudum Nov 24, 2023

chaudum Nov 24, 2023

chaudum Nov 24, 2023

chaudum Nov 24, 2023

bloom blocks downloading queue #11201

bloom blocks downloading queue #11201

Conversation

vlad-diachenko commented Nov 10, 2023

owen-d left a comment

Choose a reason for hiding this comment

github-actions bot commented Nov 21, 2023 • 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

chaudum left a 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

github-actions bot commented Nov 21, 2023 •

edited

Loading