Skip to content
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

[enhancement] querier cache: WriteBackCache should be off query path #5083

Merged
merged 10 commits into from
Jan 11, 2022

Conversation

liguozhong
Copy link
Contributor

@liguozhong liguozhong commented Jan 10, 2022

ref issue: #5072

a queue (buffered channel) configurable in size (500 keys ~ 1GB)

`c.asyncQueue = make(chan []Chunk, c.maxAsyncBufferSize)
maxAsyncBufferSize:  1000,`

Each chunk is 1Mb, and the chan length is 1000, which is approximately equal to 1Gb

@liguozhong liguozhong requested a review from a team as a code owner January 10, 2022 05:36
@liguozhong
Copy link
Contributor Author

@cyriltovena review pr please.

pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking better, thank you @liguozhong 👍
A couple more minor comments to resolve and we can merge this.

pkg/storage/chunk/cache/cache.go Outdated Show resolved Hide resolved
Co-authored-by: Danny Kopping <dannykopping@gmail.com>
@liguozhong
Copy link
Contributor Author

thanks

@cyriltovena
Copy link
Contributor

cyriltovena commented Jan 10, 2022

Can we add 2 metrics , enqueue total and dequeue total ? The only reason for skipping is that the buffer is full also so I would commit and make the metric name related to it as well as drop the label reason.

Love it though thank you for this @liguozhong

@liguozhong
Copy link
Contributor Author

Can we add 2 metrics , enqueue total and dequeue total ? The only reason for skipping is that the buffer is full also so I would commit and make the metric name related to it as well as drop the label reason.

Love it though thank you for this @liguozhong

done
image

Copy link
Contributor

@dannykopping dannykopping left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last minor notes from me and then I think we're good to go 👌

pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
pkg/storage/chunk/cache/cache.go Outdated Show resolved Hide resolved
pkg/storage/chunk/chunk_store_utils.go Outdated Show resolved Hide resolved
Copy link
Contributor

@sandeepsukhani sandeepsukhani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @liguozhong for the PR! I added just a minor nit, but other than that, the changes look great to me, modulo what Danny suggested.

decodeRequests: make(chan decodeRequest),
maxAsyncConcurrency: maxAsyncConcurrency,
maxAsyncBufferSize: maxAsyncBufferSize,
stop: make(chan struct{}, 1),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to make this a buffered channel since you are just using it to notify goroutines to stop by closing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done,thanks .

Copy link
Member

@owen-d owen-d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a few outstanding suggestions, but overall this looks great. Nice work and thanks!

func (c *Fetcher) writeBackCacheAsync(fromStorage []Chunk) error {
select {
case c.asyncQueue <- fromStorage:
chunkFetcherCacheQueueEnqueue.Inc()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chunkFetcherCacheQueueEnqueue.Add(len(fromStorage))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done,thanks .

for {
select {
case fromStorage := <-c.asyncQueue:
chunkFetcherCacheQueueDequeue.Inc()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chunkFetcherCacheQueueDequeue.Add(-len(fromStorage))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done,thanks .

@liguozhong
Copy link
Contributor Author

done, thanks all
image

@dannykopping
Copy link
Contributor

Awesome contribution @liguozhong ❤️ thanks for being patient with us with the changes requested in the review.

@dannykopping dannykopping merged commit 29cccbe into grafana:main Jan 11, 2022
slim-bean added a commit that referenced this pull request Apr 3, 2024
Signed-off-by: Edward Welch <edward.welch@grafana.com>
slim-bean added a commit that referenced this pull request Apr 4, 2024
Signed-off-by: Edward Welch <edward.welch@grafana.com>
slim-bean added a commit that referenced this pull request Apr 4, 2024
Signed-off-by: Edward Welch <edward.welch@grafana.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants