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.

Sign up for GitHub

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

Jump to bottom

etcdserver: handle watch timeouts and streaming #1138

Merged
merged 3 commits into from
Sep 24, 2014
Merged

etcdserver: handle watch timeouts and streaming #1138

merged 3 commits into from
Sep 24, 2014

Conversation

jonboulle
Copy link
Contributor 

; time curl localhost:4001/v2/keys?wait=true
context deadline exceeded

real    0m10.009s
user    0m0.004s
sys 0m0.001s

@jonboulle jonboulle added this to the v0.5.0-RC milestone Sep 23, 2014
@jonboulle
Copy link
Contributor Author

Putting this up for discussion as I'm not actually sure on the best way we should handle this. (5 minutes is taken from this: 084dcb5#diff-6ea3031fec2b1a1ef7ff4f657c95d88fR299)

Previously we just abort the connection with EOF when the timeout is reached - should we keep doing that?

@jonboulle
Copy link
Contributor Author

derr, I just realised another big difference in watch from 0.4.x - we aren't sending any HTTP headers until receiving a response from etcdserver - whereas in 0.4.x we sent a 200 OK + other headers immediately, only blocking on the body.

@xiangli-cmu @unihorn is this expected/intentional?

@yichengq
Copy link
Contributor

@jonboulle : context for quickly returning header: #877

i think it should return Transfer-Encoding: chunked at the beginning of the response. (https://trac.tools.ietf.org/html/rfc6202#section-3.1)
And rfc says that we should give out '0' to end the connection.

@jonboulle
Copy link
Contributor Author

@unihorn thanks for the reference, totally missed that. Not sure why this was not implemented in this code to begin with..

I'll revisit this patch, stand by.

@jonboulle jonboulle force-pushed the 1138_timeout branch 2 times, most recently from e2e05b7 to f3ed6c5 Compare September 23, 2014 21:09
@jonboulle
Copy link
Contributor Author

OK, updated to re-introduce streaming and respect the RFC.

@jonboulle jonboulle changed the title Context deadline times out watches etcdserver: handle watch timeouts and streaming Sep 23, 2014
@jonboulle
Copy link
Contributor Author

stand by, more changes incoming

@jonboulle
Copy link
Contributor Author

ok, here goes - handleWatch needs a test, but other than that, ready for review.

jonboulle
jonboulle reviewed Sep 23, 2014
View reviewed changes
etcdserver/etcdhttp/http.go
// Ensure headers are flushed early, in case of long polling
w.(http.Flusher).Flush()

cw := httputil.NewChunkedWriter(w)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This implies a divergence from 0.4.x behaviour, but afaict 0.4.x behaviour was quite broken.

Specifically, watches now properly obey the streaming spec - writing the content length and carriage returns/newlines for each event.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arguably we should actually differentiate here between streaming/non-streaming cases and only use a ChunkedWriter for the former.. But in that case, we would need to define the behaviour of a non-streamed watch timing out - what gets sent to the client? Thoughts?

@jonboulle
Copy link
Contributor Author

@xiangli-cmu I think we should probably just support stream for backwards compatibility since it's a very small maintenance overhead (at least that's how it looks to me). Then we could re-evaluate for API v3 if you want.

@jonboulle jonboulle self-assigned this Sep 24, 2014
xiang90
xiang90 reviewed Sep 24, 2014
View reviewed changes
etcdserver/etcdserverpb/etcdserver.pb.go
@@ -14,17 +14,16 @@
package etcdserverpb

import proto "github.com/coreos/etcd/third_party/code.google.com/p/gogoprotobuf/proto"
import json "encoding/json"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jonboulle Wrong gogoprotobuf version. you have to reset the head. :(

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@xiangli-cmu I reset to the SHA you gave me.... can you double check?

@xiang90
Copy link
Contributor

xiang90 commented Sep 24, 2014

blocked on the protobuf problem.

@jonboulle
etcdserver: correct timeout and streaming handling
172a32e 
This reintroduces the 'stream' parameter to support long-lived watch
sessions. These sessions respect a server timeout (set to 5 minutes by
default).
@jonboulle
etcdserver: use Context to communicate timeout, and add tests
ddc30c0
@jonboulle
etcdhttp: add test for streaming watches
a9caa24
@jonboulle
Copy link
Contributor Author

Fixed protobufs.

@xiang90
Copy link
Contributor

xiang90 commented Sep 24, 2014

lgtm

jonboulle added a commit that referenced this pull request Sep 24, 2014
@jonboulle
Merge pull request #1138 from jonboulle/1138_timeout
ec1df42 
etcdserver: handle watch timeouts and streaming
@jonboulle jonboulle merged commit ec1df42 into etcd-io:master Sep 24, 2014
@jonboulle jonboulle deleted the 1138_timeout branch September 24, 2014 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jonboulle jonboulle

Labels
type/bug
Milestone
v0.5.0-alpha
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jonboulle @yichengq @xiang90