Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(90)

Issue 24040044: apiserver: analyzing ping timeouts

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by mue
Modified:
10 years, 4 months ago
Reviewers:
mp+194862, fwereade, rog
Visibility:
Public.

Description

apiserver: analyzing ping timeouts This is a first fix of #1205451 handling externally killed juju nodes. Currently the pings of clients to the API server are not analyzed. With this change each ping leads to the reset of a timer waiting for 3 minutes. If it is not reset (by the ping) the timer signal tells the pinger to kill the connection. The overall problem with the presence pinger, especially regarding to HA environments, is not covered by this change. https://code.launchpad.net/~themue/juju-core/055-apiserver-heartbeat-analyzing/+merge/194862 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 18

Patch Set 2 : apiserver: analyzing ping timeouts #

Total comments: 14

Patch Set 3 : apiserver: analyzing ping timeouts #

Total comments: 14

Patch Set 4 : apiserver: analyzing ping timeouts #

Unified diffs Side-by-side diffs Delta from patch set Stats (+133 lines, -14 lines) Patch
A [revision details] View 1 2 3 1 chunk +2 lines, -0 lines 0 comments Download
M state/apiserver/admin.go View 1 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/export_test.go View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/apiserver/root.go View 1 2 3 5 chunks +102 lines, -13 lines 0 comments Download
M state/apiserver/root_test.go View 1 2 3 2 chunks +26 lines, -0 lines 0 comments Download

Messages

Total messages: 9
mue
Please take a look.
10 years, 4 months ago (2013-11-12 13:40:59 UTC) #1
rog
This is going in a reasonable direction, but I have a few comments to make. ...
10 years, 4 months ago (2013-11-12 16:01:21 UTC) #2
fwereade
Thanks rog -- just emphasising a couple of your points. https://codereview.appspot.com/24040044/diff/1/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/24040044/diff/1/state/apiserver/root.go#newcode251 ...
10 years, 4 months ago (2013-11-14 15:10:12 UTC) #3
mue
Please take a look. https://codereview.appspot.com/24040044/diff/1/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/24040044/diff/1/state/apiserver/root.go#newcode251 state/apiserver/root.go:251: pinger, err := newSrvPinger(r, 3*time.Minute) ...
10 years, 4 months ago (2013-11-18 15:31:06 UTC) #4
rog
Not quite there yet. Some thoughts below. https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode44 state/apiserver/root.go:44: initialRoot: root, ...
10 years, 4 months ago (2013-11-18 18:19:47 UTC) #5
mue
Please take a look. https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/24040044/diff/20001/state/apiserver/root.go#newcode44 state/apiserver/root.go:44: initialRoot: root, On 2013/11/18 18:19:48, ...
10 years, 4 months ago (2013-11-19 13:50:46 UTC) #6
rog
> Please describe "... connected somehow to the client ping logic" more. Shall > different ...
10 years, 4 months ago (2013-11-19 14:44:07 UTC) #7
mue
Please take a look. https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go File state/apiserver/root.go (right): https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go#newcode33 state/apiserver/root.go:33: const timeout = 3 * ...
10 years, 4 months ago (2013-11-19 15:52:37 UTC) #8
rog
10 years, 4 months ago (2013-11-20 12:31:10 UTC) #9
On 2013/11/19 15:52:37, mue wrote:
> Please take a look.
> 
> https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go
> File state/apiserver/root.go (right):
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go#ne...
> state/apiserver/root.go:33: const timeout = 3 * time.Minute
> On 2013/11/19 14:44:08, rog wrote:
> > s/timeout/maxPingInterval/
> > ?
> > (more obvious what the timeout is)
> > 
> > I think I'd define it inside state/api. Then the api client
> > code can use it to calculate an appropriate ping interval.
> 
> Renamed it and made a comment. I would move it not earlier than the according
> API changes which could be done in another CL.
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go#ne...
> state/apiserver/root.go:317: // that invokes the given action if there is more
> On 2013/11/19 14:44:08, rog wrote:
> > s/action/action asynchronously/
> 
> Done.
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root.go#ne...
> state/apiserver/root.go:329: go pt.action()
> On 2013/11/19 14:44:08, rog wrote:
> > I don't think we want to call this if stop has been called.
> > when I said "below" I meant in the <-timer.C arm of the select statement.
> 
> Ah, already wondered.
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root_test.go
> File state/apiserver/root_test.go (right):
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root_test....
> state/apiserver/root_test.go:53: for i := 0; i < 10; i++ {
> On 2013/11/19 14:44:08, rog wrote:
> > s/10/2/
> > 
> > i don't think there's really any particular reason to make this test run any
> > longer than necessary.
> 
> Done.
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root_test....
> state/apiserver/root_test.go:60: closed := <-closedc
> On 2013/11/19 14:44:08, rog wrote:
> > select {
> > case closed = <-closed:
> > case <-testing.LongWait:
> >     c.Fatalf("action never executed")
> > }
> > 
> > otherwise the test will hang forever if the action never
> > gets called.
> 
> Had luck so far, but that's better.
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root_test....
> state/apiserver/root_test.go:61: closeDiff := closed.Sub(broken).Nanoseconds()
/
> 1000000
> On 2013/11/19 14:44:08, rog wrote:
> > or:
> > closeDiff := closed.Sub(broken) / time.Millisecond
> 
> Gnah! Sure. :)
> 
>
https://codereview.appspot.com/24040044/diff/40001/state/apiserver/root_test....
> state/apiserver/root_test.go:62: c.Assert(closeDiff >= 50 && closeDiff <= 60,
> gc.Equals, true)
> On 2013/11/19 14:44:08, rog wrote:
> > 50 <= closeDiff && closeDiff <= 60
> > 
> > is a nice convention for range checks.
> > 
> > I'd also make the allowable range much larger to cater
> > for dubious scheduling in the cloud, so that this doesn't
> > break too often.
> > 
> > perhaps 50 <= closeDiff && closeDiff <= 100
> 
> Done.

LGTM, thanks.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b