Skip to content
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

clientv3: fix keepalive send interval when response queue is full #9952

Merged
merged 2 commits into from Jul 23, 2018

Conversation

@gyuho
Copy link
Member

@gyuho gyuho commented Jul 22, 2018

client should update next keepalive send time
even when lease keepalive response queue becomes full.

Otherwise, client sends keepalive request every 500ms
regardless of TTL when the send is only expected to happen
with the interval of TTL / 3 at minimum.

Fix #9911.

/cc @cfc4n

@codecov-io
Copy link

@codecov-io codecov-io commented Jul 22, 2018

Codecov Report

Merging #9952 into master will increase coverage by 0.1%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #9952     +/-   ##
=========================================
+ Coverage   69.12%   69.23%   +0.1%     
=========================================
  Files         386      386             
  Lines       35784    35789      +5     
=========================================
+ Hits        24735    24777     +42     
+ Misses       9232     9213     -19     
+ Partials     1817     1799     -18
Impacted Files Coverage Δ
clientv3/lease.go 91.85% <100%> (+0.53%) ⬆️
pkg/adt/interval_tree.go 84.98% <0%> (-5.71%) ⬇️
integration/bridge.go 70.22% <0%> (-1.53%) ⬇️
etcdserver/raft.go 80.09% <0%> (-0.72%) ⬇️
etcdserver/api/rafthttp/stream.go 78.96% <0%> (-0.43%) ⬇️
clientv3/watch.go 92.14% <0%> (ø) ⬆️
etcdserver/v3_server.go 73.65% <0%> (+0.23%) ⬆️
pkg/proxy/server.go 60.2% <0%> (+0.33%) ⬆️
raft/raft.go 91.92% <0%> (+0.69%) ⬆️
raft/node.go 91.63% <0%> (+0.79%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 104b6a3...e93fb56. Read the comment docs.

// expect lease keepalive every 10-second
lresp, err := lapi.Grant(context.Background(), 30)
if err != nil {
t.Errorf("failed to create lease %v", err)
Copy link
Contributor

@xiang90 xiang90 Jul 22, 2018

you should switch t.errorf and t.fatelf

errorf is to report errors in test, which does not match the test case expectation.

fatalf is to report errors that are not tested in the case, but are failing.

@xiang90
Copy link
Contributor

@xiang90 xiang90 commented Jul 22, 2018

lgtm

gyuho added 2 commits Jul 23, 2018
client should update next keepalive send time
even when lease keepalive response queue becomes full.

Otherwise, client sends keepalive request every 500ms
regardless of TTL when the send is only expected to happen
with the interval of TTL / 3 at minimum.

Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
Signed-off-by: Gyuho Lee <gyuhox@gmail.com>
@gyuho gyuho force-pushed the fix-keepalive branch from a876165 to e93fb56 Jul 23, 2018
@xiang90 xiang90 merged commit e4e3471 into etcd-io:master Jul 23, 2018
2 checks passed
@gyuho gyuho deleted the fix-keepalive branch Jul 23, 2018
@cfc4n
Copy link
Contributor

@cfc4n cfc4n commented Jul 24, 2018

Sorry, I'm late.thanks for fix. @gyuho

@gyuho
Copy link
Member Author

@gyuho gyuho commented Jul 24, 2018

@cfc4n Thanks for report. This will be backported and released with patch release.

luoyancn added a commit to luoyancn/dubhe that referenced this issue Aug 11, 2018
When use etcd keepalive method, we must consume all messages
from the queen it retured.Otherwise, the queen would be full.
See more detail, visit the follow links:

etcd-io/etcd#9952
etcd-io/etcd#8168
https://github.com/coreos/etcd/blob/master/clientv3/concurrency/session.go#L63
https://www.cnxct.com/etcd-lease-keepalive-debug-note/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants