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
Store entire ring under single key in Consul. #24
Conversation
3b68b46
to
2e0f84d
Compare
2e0f84d
to
95be339
Compare
Get(key string, factory InstanceFactory) error | ||
CAS(key string, factory InstanceFactory, f CASCallback) error | ||
WatchPrefix(path string, factory InstanceFactory, done chan struct{}, f func(string, interface{}) bool) | ||
WatchKey(key string, factory InstanceFactory, done chan struct{}, f func(interface{}) bool) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the doc comment with a note about InstanceFactory
.
populateRingDesc(ringDesc, r.id, r.hostname, tokens) | ||
|
||
return ringDesc, true, nil | ||
}); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This took me quite a while to figure out. Here are some things that I think might help make it easier to follow:
- Separate assigning to
err
from theerr != nil
check - Define the function being passed to
CAS
separately and give it a name. Maketokens
a parameter of that function and close overtokens
here.
Done my first pass review. I'll wait until you give me a heads up before doing a second pass. Looks really good though! |
maxBackoff = 1 * time.Minute | ||
) | ||
var ( | ||
backoff = initialBackoff / 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be / 2
What's your testing strategy for this? |
A few more questions. I promise the final round of review will be very quick. |
I've run this all locally and tested killing all the various components in various orders. |
LGTM, thanks. In a future PR, we should automate those tests you did. |
Part of #8