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

ring.DoBatch: check context cancellation #454

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

colega
Copy link
Contributor

@colega colega commented Dec 20, 2023

What this PR does:

This changes ring.DoBatch & ring.DoBatchWithOptions to check the context cancellation, as it might take a while to calculate the instances for all keys and sometimes it doesn't make sense to proceed with the callbacks.

Which issue(s) this PR fixes:

None.

Checklist

  • Tests updated
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

This changes ring.DoBatch & ring.DoBatchWithOptions to check the context
cancellation, as it might take a while to calculate the instances for
all keys and sometimes it doesn't make sense to proceed with the
callbacks.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
@colega colega marked this pull request as ready for review December 20, 2023 10:20
colega added a commit to grafana/mimir that referenced this pull request Dec 20, 2023
It may take a while to calculate which instance gets each one of the
series (see: grafana/dskit#454)

It isn't fair (and is useless) to reach the callback with no ttl.

This changes the code to start the timeout once the first callback is
called.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Copy link
Contributor

@duricanikolic duricanikolic left a comment

Choose a reason for hiding this comment

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

LGTM
with a small nit

ring/ring_test.go Outdated Show resolved Hide resolved
ring/ring_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov left a comment

Choose a reason for hiding this comment

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

LGTM, makes sense

@colega colega merged commit e72c4ad into main Dec 20, 2023
3 checks passed
@colega colega deleted the ring-do-batch-with-options-checking-context branch December 20, 2023 17:06
colega added a commit to grafana/mimir that referenced this pull request Dec 21, 2023
* Distributor: start remote timeout on first callback

It may take a while to calculate which instance gets each one of the
series (see: grafana/dskit#454)

It isn't fair (and is useless) to reach the callback with no ttl.

This changes the code to start the timeout once the first callback is
called.

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Update CHANGELOG.md

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

* Refactor using sync.OnceValues

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>

---------

Signed-off-by: Oleg Zaytsev <mail@olegzaytsev.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants