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

StreamItem InvocationId cleanup #5315

Closed
DylanDmitri opened this issue Jul 3, 2018 · 7 comments
Closed

StreamItem InvocationId cleanup #5315

DylanDmitri opened this issue Jul 3, 2018 · 7 comments
Assignees
Labels
area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@DylanDmitri
Copy link
Contributor

Previously, stream items were returned from the server in response to an invocation, and so carried an invocation id for routing purposes.

Now, stream items can be sent to the server, and are sorted based on stream id. Regardless of direction, all id values are stored under the InvocationId field on StreamItem. This is poor naming and rather misleading.

Possible solutions include:

  • StreamItem has two fields, StreamId and InvocationId, only one of which can be filled at a time
  • StreamItem has one field, RoutingId, which is used both ways. This breaks backwards compatibility.
  • Make a new class StreamParameterItem that's identical to stream item but has a StreamId field instead.

None of these solutions feels very clean.

@DylanDmitri DylanDmitri self-assigned this Jul 3, 2018
@analogrelay
Copy link
Contributor

analogrelay commented Jul 4, 2018

This one is going to take some extra thought so I'm increasing the cost for now. If it comes in under cost, that's even better :)

@DylanDmitri
Copy link
Contributor Author

There's more thinking to do... the same base problem of InvocationId -vs- StreamId also applies to

  • InvocationBindingFailureMessage (currently one class, probably should make a StreamItemBindingFailureMessage)
  • CompletionMessage vs StreamCompleteMessage

Want to consider all of the cases when deciding on a good pattern.

@DylanDmitri
Copy link
Contributor Author

Just making two classes for each. It seems the flattest/cleanest even if it's not concise/elegant.

@BrennanConroy
Copy link
Member

@halter73 @mikaelm12
We are wanting to do a little redesign here. Idea is to move back to a single StreamItem message and CompletionMessage.

We have a bit of flexibility if we change the protocol version so we can have StreamItem with streamId in the new protocol and invocationId in the old protocol.

@muratg
Copy link
Contributor

muratg commented Dec 7, 2018

#4401

@BrennanConroy BrennanConroy self-assigned this Dec 10, 2018
@aspnet-hello aspnet-hello transferred this issue from aspnet/SignalR Dec 17, 2018
@aspnet-hello aspnet-hello added this to the 3.0.0 milestone Dec 17, 2018
@aspnet-hello aspnet-hello added area-signalr Includes: SignalR clients and servers cost: M labels Dec 17, 2018
@BrennanConroy BrennanConroy modified the milestones: 3.0.0, 3.0.0-preview2 Jan 11, 2019
@BrennanConroy
Copy link
Member

Done #4559

@davidfowl
Copy link
Member

Thanks @DylanDmitri ! oh and @BrennanConroy 😉

@analogrelay analogrelay added release-3.0 enhancement This issue represents an ask for new feature or an enhancement to an existing one and removed enhancement This issue represents an ask for new feature or an enhancement to an existing one type: Enhancement labels Mar 21, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-signalr Includes: SignalR clients and servers enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

6 participants