Skip to content

Use dynamic response channels to avoid responses going to every client #409

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

Conversation

doodlesbykumbi
Copy link

@doodlesbykumbi doodlesbykumbi commented May 18, 2021

Resolves #407

Dynamic response channels are subscribed to, by the request-originator, just before the request is dispatched. This ensures that responses are consumed only by the originator, avoiding unnecessary noise for the other clients.

Dynamic response channels are subscribed to by the request-originator just before the request is dispatched. This ensures that responses are consumed only the originator, avoiding unnecessary noise for the other clients.
@doodlesbykumbi doodlesbykumbi force-pushed the dynamic-response-channel branch from 2fcb377 to b067e51 Compare May 19, 2021 13:44
@doodlesbykumbi doodlesbykumbi marked this pull request as ready for review May 19, 2021 15:13
@y4my4my4m
Copy link

I haven't tested it but, if this works as described, would be a great performance boost!

@doodlesbykumbi are the dynamic response automatic or do you need to modify server code to keep track of subscribed channels?

@doodlesbykumbi
Copy link
Author

doodlesbykumbi commented May 25, 2021

@y4my4my4m The dynamic response is automatic. The idea is

  1. Client creates request and subscribes to response channel (prefix + "-response#" + this.nsp.name + "#" + requestId)
  2. Client sends request
  3. Client either times out or gets all the responses it needs
  4. Client unsubscribes from response channel

It's a mostly transparent change for consumers of the adapter. It could be made entirely transparent by having it be backwards compatible, but I think it's better to just go all in.

@darrachequesne
Copy link
Member

@doodlesbykumbi thanks for the pull request! If I understand correctly, this change is not backward compatible and will require a synchronized cluster update, isn't it?

@doodlesbykumbi
Copy link
Author

@darrachequesne Yes, the current change is entirely backwards incompatible. I think there was a way to maintain backwards compatibility but I opted against it.

@doodlesbykumbi
Copy link
Author

I think to maintain backwards compatibility just before dispatching requests we would also have to listen on the original response channel. When we publish a response it would also have to be published on the original response channel.

An edge case (for when dispatching requests) I have to figure out is what happens when by some luck multiple requests take place at the same time. I suppose we'll have to keep a count, and only clean up when the count goes back to 0

@darrachequesne
Copy link
Member

Superseded by f66de11.

@darrachequesne darrachequesne added this to the 7.1.0 milestone Nov 30, 2021
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.

request-response cycle does NxN work when it could do Nx1
3 participants