Fix retries to create consul client #610
Conversation
|
Interesting. Looks like jenkins isn't using |
There was a problem hiding this comment.
Hei @fdawg4l , thanks for the PR. You are missing a Signed-off-by on your last commit. It would also be nice if your commit message started with a topic prefix like kvstore: or consul:. See previous commits for examples.
pkg/kvstore/consul_test.go
Outdated
| handler = func(w http.ResponseWriter, r *http.Request) { | ||
| triesAttempted++ | ||
| io.WriteString(w, "\"nanananananananananleaaderrrr\"") | ||
| } |
There was a problem hiding this comment.
What does nanananananananananleaaderrrr mean? Please change it to something more meaningful.
There was a problem hiding this comment.
It is just a test string. And it's from The Simpsons. Because leader election... :-). https://www.youtube.com/watch?v=1aYN5XpWzpM
There was a problem hiding this comment.
Changes look great. A SOB would be great though. See https://github.com/cilium/cilium/blob/master/Documentation/contributing.rst#developers-certificate-of-origin
|
I'll squash my commits, include the SOA, and add the |
|
Interesting; the test failed. It seems buggy. I'll take a look. Please standby. Apologies! |
The retry loop in NewConsulClient in kvstore was off by one. Added test to test the retry behavior and a successful connection using a mock server. Signed-off-by: Faiyaz Ahmed <faiyaza@gmail.com>
|
I couldn't repro locally, and to be honest i have no idea why it failed. It smells like it's because the handler is run asynchronously, but the error is only returned by the client after the handler runs. So the error collected (and tries count increment) should always happen in order. But it still failed. In any case, i refactored it with a channel which is only closed after ...This change really wasn't suppose to be such a saga. Apologies :-). And thanks! |
Awesome work. Thanks! |
Related-to: #610 (Fix retries to create consul client) Signed-off-by: Alexander Alemayhu <alexander@alemayhu.com>
The retry loop in NewConsulClient in kvstore was off by one. Added test to test the
retry behavior and a successful connection using a mock server.