Skip to content

Implement Hashicorps's recommendations when watching consul key #1713

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 14 commits into from
Oct 22, 2019
Merged

Implement Hashicorps's recommendations when watching consul key #1713

merged 14 commits into from
Oct 22, 2019

Conversation

pstibrany
Copy link
Contributor

@pstibrany pstibrany commented Oct 3, 2019 •
edited
Loading

This PR implements Hashicorp's recommendations for using blocking queries at https://www.consul.io/api/features/blocking.html:

  • rate limiting when watching for value changes. Current limit is 1 read/second, with bursts up to 5/second.
  • handle LastIndex value going down or being zero by restarting the blocking loop from 0 or 1 respectively

Fixes issue #1708.

@pstibrany pstibrany changed the title Issue 1708 Implement Hashicorps's recommendations when watching consul key Oct 3, 2019
khaines
khaines reviewed Oct 7, 2019
View reviewed changes
Copy link
Contributor

@khaines khaines left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @pstibrany. It's a pity we had to fork the rate limiter code vs just referencing it, but other than that it LGTM

@pstibrany
Copy link
Contributor Author

Thanks for your review.

I couldn't find any other fitting rate limiter in dependencies that Cortex already has, and I didn't want to introduce new dependency. I'm open to switching to a different one if you have a good suggestion, although I quite like how simple this one is.

@tomwilkie
Copy link
Contributor

tomwilkie commented Oct 8, 2019 •
edited
Loading

@pstibrany have you seen https://godoc.org/golang.org/x/time/rate?

ps we already use it here: https://github.com/cortexproject/cortex/blob/master//pkg/distributor/distributor.go#L403

@pstibrany
Copy link
Contributor Author

Replaced with rate limiter from golang.org/x/time/rate. Not as nice, but it works and we avoid copying.

tomwilkie
tomwilkie reviewed Oct 8, 2019
View reviewed changes
@pstibrany
Copy link
Contributor Author

pstibrany commented Oct 8, 2019 •
edited
Loading

Oh, I guess rate-limit should apply for WatchPrefix as well, not just WatchKey. I missed that.
And not just rate-limit, but index checks too.

gouthamve
gouthamve approved these changes Oct 18, 2019
View reviewed changes
Copy link
Contributor

@gouthamve gouthamve left a comment

Choose a reason for hiding this comment

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

Looks good, left some comments.

pracucci
pracucci reviewed Oct 18, 2019
View reviewed changes
jtlisi
jtlisi approved these changes Oct 22, 2019
View reviewed changes
Copy link
Contributor

@jtlisi jtlisi left a comment

Choose a reason for hiding this comment

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

LGTM

@gouthamve
Copy link
Contributor

@pstibrany Could you add a changelog entry and I'll merge this after that and Jacob's nit.

@pstibrany
Implemented recommendations from Consul docs
a4edf55 
https://www.consul.io/api/features/blocking.html#implementation-details

Fixes issue #1708

Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Added tests for rate limiting, and index resets.
9a2c81d 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Call cancel function, even though we don't use it.
549a578 
This makes lint happy.

Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Use rate limiter from golang.org/x/time/rate
3a40ebd 
Removed copy of modified jaegers' rate-limiter

Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Rate limit configuration
a69efb8 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
WatchPrefix should use same index checks as WatchKey
0903003 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Added comment for NewInMemoryClientWithConfig
9427ab2 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Use rate.Inf as rate limit, when it is not set.
429e92b 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
TestReset doesn't use rate limit now.
1e53278 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Remove unused variable.
ef0df6d 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Rename watch-rate -> watch-rate-limit, watch-burst -> watch-burst-size.
638470b 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Use 0 instead of "Zero"
e756dd2 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Updated changelog with consul client enhancements.
0d87f38 
Signed-off-by: Peter Štibraný <[email protected]>
@pstibrany
Copy link
Contributor Author

Added changelog, fixed "Zero" -> "0", and rebased on top of master.

gouthamve
gouthamve reviewed Oct 22, 2019
View reviewed changes
@gouthamve gouthamve merged commit b80d1f6 into cortexproject:master Oct 22, 2019
@pstibrany pstibrany deleted the issue-1708 branch October 22, 2019 07:49
@gouthamve gouthamve mentioned this pull request Oct 24, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
5 more reviewers

@tomwilkie tomwilkie tomwilkie left review comments

@khaines khaines khaines left review comments

@pracucci pracucci pracucci left review comments

@gouthamve gouthamve gouthamve approved these changes

@jtlisi jtlisi jtlisi approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pstibrany @tomwilkie @gouthamve @khaines @pracucci @jtlisi