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

[ADDED] Support for Websocket protocol #719

Merged
merged 3 commits into from Apr 27, 2021
Merged

[ADDED] Support for Websocket protocol #719

merged 3 commits into from Apr 27, 2021

Conversation

kozlovic
Copy link
Member

Websocket protocol will be used as long as the URL scheme is ws or wss.
Note that no mixing of websocket and non-websocket URLs can be used (in the list of servers). Once a client connects, the gossip will be augmenting the URLs of that "type".

A Compression() option was added, which is currently documented as being used only for Websocket connections.

This is in preparation before adding WS support.

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
Copy link
Member

@derekcollison derekcollison left a comment

Choose a reason for hiding this comment

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

LGTM - Like the way you have organized things, will serve us well IMO.

Any data on performance changes?

@ripienaar
Copy link
Contributor

Very exciting :)

@ripienaar
Copy link
Contributor

LGTM, also curious about performance

@kozlovic
Copy link
Member Author

Not sure if you are asking about performance of WS or impact of refactoring on non WS. For the later, quick dirty test.

From master:

$ go run examples/nats-bench/main.go -np 10 -ns 1 -n 10000000 foo
Starting benchmark [msgs=10000000, msgsize=128, pubs=10, subs=1]
NATS Pub/Sub stats: 5,511,378 msgs/sec ~ 672.78 MB/sec
 Pub stats: 2,755,819 msgs/sec ~ 336.40 MB/sec
..
 Sub stats: 2,755,838 msgs/sec ~ 336.41 MB/sec

From ws branch:

$ go run examples/nats-bench/main.go -np 10 -ns 1 -n 10000000 foo
Starting benchmark [msgs=10000000, msgsize=128, pubs=10, subs=1]
NATS Pub/Sub stats: 5,595,547 msgs/sec ~ 683.05 MB/sec
 Pub stats: 2,798,012 msgs/sec ~ 341.55 MB/sec
...
 Sub stats: 2,798,307 msgs/sec ~ 341.59 MB/sec

So really no impact on this test.
For Websocket perf:

$ go run examples/nats-bench/main.go -s ws://127.0.0.1:8080 -np 10 -ns 1 -n 10000000 foo
Starting benchmark [msgs=10000000, msgsize=128, pubs=10, subs=1]
NATS Pub/Sub stats: 4,921,103 msgs/sec ~ 600.72 MB/sec
 Pub stats: 2,460,663 msgs/sec ~ 300.37 MB/sec
...
 Sub stats: 2,460,640 msgs/sec ~ 300.37 MB/sec

@ripienaar
Copy link
Contributor

Amazing.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.8%) to 86.957% when pulling 995f671 on ws into 6cee9f0 on master.

@derekcollison
Copy link
Member

That's awesome.

Copy link
Member

@ColinSullivan1 ColinSullivan1 left a comment

Choose a reason for hiding this comment

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

LGTM. Nice!

ws.go Outdated Show resolved Hide resolved
@mirusky
Copy link

mirusky commented Apr 25, 2021

Hi guys, thanks for your work.

Just some questions

  • When this will be available?
  • Or it's safe to use branch ws while this isn't merged?

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
@kozlovic kozlovic marked this pull request as ready for review April 26, 2021 14:59
@kozlovic
Copy link
Member Author

@mirusky Thanks! We actually going to likely merge this in the main branch once the review is complete and Websocket will be part of the upcoming v1.11.0 release.

@kozlovic
Copy link
Member Author

@ColinSullivan1 Please have a look at c8c0b60 and see if it helps.

@ColinSullivan1
Copy link
Member

LGTM - thanks Ivan!

@kozlovic kozlovic merged commit 109f3dd into master Apr 27, 2021
@kozlovic kozlovic deleted the ws branch April 27, 2021 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants