Skip to content

Conversation

pracucci
Copy link
Contributor

What this PR does:
This is like #2827 but for the compactor. The idea is to add the ability to wait for a stable ring at compactor startup. If this proposal is accepted, I will then work on the same for the store-gateway.

Which issue(s) this PR fixes:
N/A

Checklist

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

Copy link
Contributor

@pstibrany pstibrany left a comment

Choose a reason for hiding this comment

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

LGTM, thanks. My comments are not blocking.

pkg/ring/util.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest removing maxWaiting and rely on context only. Client calling this can already setup context deadline, if needed. The code right now returns context.DeadlineExceeded when maxWaiting is reached, which client may confuse with its own context deadline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the same, but using it from the caller perspective is more annoying, because it's not something you can do inline. You need to get a new context, defer the cancel(), call the WaitRingStability().

pkg/ring/util.go Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Use ticker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's more lines of code. Any benefit of using a ticker?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed offline. Will move to ticker.

Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
Signed-off-by: Marco Pracucci <[email protected]>
@pracucci pracucci force-pushed the wait-for-stable-ring branch from 209182b to 5b70ebd Compare November 12, 2020 09:57
@pracucci pracucci merged commit 7716491 into cortexproject:master Nov 12, 2020
@pracucci pracucci deleted the wait-for-stable-ring branch November 12, 2020 10:23
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.

2 participants