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

embed: support websocket for bi-directional streams #8257

Merged
merged 2 commits into from
Oct 7, 2017
Merged

embed: support websocket for bi-directional streams #8257

merged 2 commits into from
Oct 7, 2017

Conversation

yudai
Copy link
Contributor

@yudai yudai commented Jul 13, 2017 •
edited
Loading

For #8237

Support bi-directional streams such as Watch() by using websocket.

Let me know if I need to add tests for this change, since I couldn't find a right place to add them.

@yudai yudai force-pushed the websocket_streams branch 3 times, most recently from 2bf4f78 to 9e62a19 Compare October 5, 2017 22:40
@yudai yudai changed the title wip: support websocket for bi-directional streams embed: support websocket for bi-directional streams Oct 5, 2017
@xiang90
Copy link
Contributor

xiang90 commented Oct 5, 2017

can we split vendor and code change into two commits?

@yudai
Copy link
Contributor Author

yudai commented Oct 5, 2017

@xiang90 Done :)

@xiang90
Copy link
Contributor

xiang90 commented Oct 5, 2017

can you change the commit message of second commit? or it wont pass CI

@yudai
Copy link
Contributor Author

yudai commented Oct 5, 2017

Sure. Let me know if * is better.

@xiang90
Copy link
Contributor

xiang90 commented Oct 5, 2017

lgtm

@xiang90 xiang90 mentioned this pull request Oct 5, 2017
Closed
@xiang90
Copy link
Contributor

xiang90 commented Oct 6, 2017

@yudai sorry, one last thing, can you run https://github.com/coreos/etcd/blob/master/scripts/updatebom.sh to update the vendor license file? thank you!

@gyuho
Copy link
Contributor

gyuho commented Oct 6, 2017

Also last commit should be something like vendor: add github.com/tmc/grpc-websocket-proxy?

@yudai
Copy link
Contributor Author

yudai commented Oct 6, 2017

ok, done :)

gyuho
gyuho approved these changes Oct 6, 2017
View reviewed changes
Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm /cc @xiang90

@xiang90
Copy link
Contributor

xiang90 commented Oct 6, 2017

@yudai hm... the windows build is broken. cannot merge. can you help to fix that? see travis ci result.

@yudai
Copy link
Contributor Author

yudai commented Oct 6, 2017

@xiang90 May I update golang.org/x/sys to the latest version/
It seems Logrus, that is a dependency of grpc-websocket-proxy, requires a newer version.

sirupsen/logrus#626 (comment)

@xiang90
Copy link
Contributor

xiang90 commented Oct 6, 2017

sure.

vendor: add github.com/tmc/grpc-websocket-proxy
0cdf5b2 
Updating golang.org/x/net as well so that a new dependency
github.com/sirupsen/logrus can be compiled on Windows environments.
@yudai
Copy link
Contributor Author

yudai commented Oct 6, 2017

Updated. Hope this run passes.

@xiang90
Copy link
Contributor

xiang90 commented Oct 7, 2017

merging. thanks @yudai and @gyuho

@xiang90 xiang90 merged commit b766a26 into etcd-io:master Oct 7, 2017
@socketpair
Copy link

As far as I understand, this change allows to use watchers over websocket, right ? Where I can read documentation ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyuho gyuho gyuho approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@yudai @xiang90 @gyuho @socketpair