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

Issue 6292044: Any session event on a watcher now fires and closes the watch.

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 3 months ago by niemeyer
Modified:
8 years, 3 months ago
Reviewers:
mp+108648, dfc
Visibility:
Public.

Description

Any session event on a watcher now fires and closes the watch. Also test that closing the connection closes the watch. https://code.launchpad.net/~niemeyer/gozk/watches-die-on-reconnection/+merge/108648 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5

Patch Set 2 : Any session event on a watcher now fires and closes the watch. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -19 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M zk.go View 1 1 chunk +11 lines, -5 lines 0 comments Download
M zk_test.go View 4 chunks +20 lines, -14 lines 0 comments Download

Messages

Total messages: 3
niemeyer
Please take a look.
8 years, 3 months ago (2012-06-04 22:08:26 UTC) #1
dfc
LGTM. https://codereview.appspot.com/6292044/diff/1/zk.go File zk.go (right): https://codereview.appspot.com/6292044/diff/1/zk.go#newcode1049 zk.go:1049: // non-session watches as fatal. this comment is ...
8 years, 3 months ago (2012-06-04 22:58:59 UTC) #2
niemeyer
8 years, 3 months ago (2012-06-04 23:43:04 UTC) #3
*** Submitted:

Any session event on a watcher now fires and closes the watch.

Also test that closing the connection closes the watch.

R=dfc
CC=
https://codereview.appspot.com/6292044

https://codereview.appspot.com/6292044/diff/1/zk.go
File zk.go (right):

https://codereview.appspot.com/6292044/diff/1/zk.go#newcode1049
zk.go:1049: // non-session watches as fatal.
On 2012/06/04 22:58:59, dfc wrote:
> this comment is a little confusing; does it mean 
> 
> // .. but we take all session events that occur before a session is
established
> as errors.

Improved comment. Thanks for the poke.

https://codereview.appspot.com/6292044/diff/1/zk_test.go
File zk_test.go (right):

https://codereview.appspot.com/6292044/diff/1/zk_test.go#newcode340
zk_test.go:340: case <-time.After(3e9):
On 2012/06/04 22:58:59, dfc wrote:
> nit: 3 * time.Second
> 
> you've imported time, so you might as well make use of the constant.

I've copy & pasted that. There are tons of uses here, historical. I woudln't
like to change that right now if that's ok.
Sign in to reply to this message.

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