clientv3: Do no stop keep alive loop by server side errors #7890
Conversation
yudai
force-pushed
the
keep_ka_loop_running
branch
from
746697f
to
4fa2607
Compare
Codecov Report
@@ Coverage Diff @@
## master #7890 +/- ##
=========================================
Coverage ? 76.04%
=========================================
Files ? 332
Lines ? 26233
Branches ? 0
=========================================
Hits ? 19950
Misses ? 4867
Partials ? 1416
Continue to review full report at Codecov.
|
clientv3/client.go
Outdated
| if err == nil { | ||
| return false | ||
| } | ||
| if err != context.Canceled && err != context.DeadlineExceeded { |
There was a problem hiding this comment.
return err == context.Canceled || err == context.DeadlineExceededThere was a problem hiding this comment.
Fixed. I just wanted to return true at the end of the function (now it's false, false, true, false).
There was a problem hiding this comment.
huh? the if isn't needed, the predicate can be returned directly..
There was a problem hiding this comment.
ah! sorry, I misunderstood your first comment (I thought it was a if statement). Yes, your way is much simpler and easy to understand :) Fixed!
clientv3/lease.go
Outdated
| } | ||
|
|
||
| err = toErr(l.stopCtx, err) | ||
| if err == rpctypes.ErrNoLeader { |
There was a problem hiding this comment.
if toErr(l.stopCtx, err) == rpctypes.ErrNoLeader
yudai
force-pushed
the
keep_ka_loop_running
branch
2 times, most recently
from
5cb3418
to
cf7eac5
Compare
|
lgtm |
clientv3/client.go
Outdated
| @@ -503,3 +503,14 @@ func toErr(ctx context.Context, err error) error { | |||
| } | |||
| return err | |||
| } | |||
|
|
|||
| func canceledByCaller(ctx context.Context, err error) bool { | |||
| if ctx.Err() == nil { | |||
There was a problem hiding this comment.
if ctx.Err() == nil || err == nil {
return false
}There was a problem hiding this comment.
probably also rename ctx to stopCtx.
yudai
force-pushed
the
keep_ka_loop_running
branch
from
cf7eac5
to
aa85b0c
Compare
|
@yudai We probably want a test to cover this. It can be a different PR though. LGTM. |
|
@xiang90 Adding a test could take a little bit time. Because I think we need a mock server that returns unexpected (not in |
|
CI is fixed on master. Merging. |
For #7880