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

SockJSSocket: disable writeHandler by default #1221

Closed
vietj opened this issue Apr 8, 2019 · 8 comments · Fixed by #1704
Closed

SockJSSocket: disable writeHandler by default #1221

vietj opened this issue Apr 8, 2019 · 8 comments · Fixed by #1704
Assignees
Milestone

Comments

@vietj
Copy link
Contributor

vietj commented Apr 8, 2019

Currently the SockJSSocket registers an event bus consumer to implement the writeHandlerID header. This is similar to WebSocket and NetSocket that do provide the same feature, however those use a local consumer.

This should be changed to use a local consumer as well.

@vietj vietj added this to the 3.7.1 milestone Apr 8, 2019
@vietj
Copy link
Contributor Author

vietj commented Apr 8, 2019

@pmlopes @tsegismont what's your opinion on this ?

@sfitts
Copy link
Contributor

sfitts commented Apr 8, 2019

Personally I would prefer that the consumer not be converted to local since we use this to route data to the web socket from across the cluster. If it were to be converted we'd have to start routing messages first to the correct target member so they could be sent. Certainly possible (we do it for other use cases), but does mean that you incur 2x the bus traffic for the same send.

One function of our system is high volume event processing where clients can subscribe to events over WS, so this is a critical use case for us.

Actually what we'd probably do is just install our own cluster wide consumer that does what this one does to replace it (and just skip using the one provided by Vert.x entirely). So from perspective I guess the impact is minimal (still would rather it stay as is though).

@vietj
Copy link
Contributor Author

vietj commented Apr 9, 2019

I think this should be configurable ?

@vietj
Copy link
Contributor Author

vietj commented Apr 9, 2019

my point is that inside Vert.x websocket/netsocket use local consumer.

as you said, this feature is nothing but complicated to do yourself too.

I don't want to create regressions either if people are using too...

@sfitts
Copy link
Contributor

sfitts commented Apr 9, 2019

Making it configurable makes sense.

@vietj vietj modified the milestones: 3.7.1, 3.8.0 May 23, 2019
@slinkydeveloper
Copy link
Member

@vietj So what we want to do for this? provide a flag to choose if register local consumer or global consumer? And we made this flag default to "global consumer"?

@vietj vietj modified the milestones: 3.8.0, 3.8.1 Jul 19, 2019
@vietj vietj modified the milestones: 3.8.1, 3.8.2 Aug 23, 2019
@vietj vietj modified the milestones: 3.8.2, 3.8.3 Oct 4, 2019
@vietj vietj modified the milestones: 3.8.3, 3.8.4 Oct 22, 2019
@vietj vietj modified the milestones: 3.8.4, 3.8.5 Dec 1, 2019
@vietj vietj modified the milestones: 3.8.5, 3.8.6 Jan 23, 2020
@vietj vietj modified the milestones: 3.8.6, 3.9.0 Feb 20, 2020
@vietj vietj modified the milestones: 3.9.0, 3.9.1 Mar 30, 2020
@tsegismont tsegismont removed this from the 3.9.1 milestone May 14, 2020
@tsegismont
Copy link
Contributor

tsegismont commented May 14, 2020

@vietj on one hand, the use-case that @sfitts describes sounds quite compelling to me. On the other hand, I believe it's a waste of resources to register a consumer, even local, any time a netsocket, websocket or sockjssocket is created, because most apps don't use it.

Consequently, I think we should add new options in Vert.x 4. Users like @sfitts who do need this feature could activate it, and choose whether or not they want the consumer to be local-only.

@tsegismont tsegismont added this to the 4.0.0 milestone May 14, 2020
@tsegismont tsegismont self-assigned this May 14, 2020
@vietj
Copy link
Contributor Author

vietj commented May 15, 2020

agreed, it should not be the default in 4

@tsegismont tsegismont changed the title SockJSSocket should register a local consumer SockJSSocket: disable writeHandler by default Sep 18, 2020
@tsegismont tsegismont linked a pull request Sep 18, 2020 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

4 participants