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

RequestReceived should contain priority information #234

Closed
Kriechi opened this issue May 30, 2016 · 8 comments
Closed

RequestReceived should contain priority information #234

Kriechi opened this issue May 30, 2016 · 8 comments
Assignees

Comments

@Kriechi
Copy link
Member

Kriechi commented May 30, 2016

A follow-up on #229 and #233:

It would be nice if the RequestReceived event contains priority information, if the HEADERS frame carries them. This way all available information about a new request is in the same event. PriorityUpdated is then only used for actual PRIORITY frames.

@Lukasa
Copy link
Member

Lukasa commented May 30, 2016

So I'm not sure exactly how we want to do this yet.

We could put the priority information in RequestReceived, but that means that priority information isn't obtained isomorphically regardless of the source: instead it needs to be found in different places depending on how the information was sent. I don't think I love that much.

I think I'd rather we decided on a solution for #228 that allows understanding this information without forcing people to know about it if they just want to handle the priority information wherever it came from.

@Kriechi
Copy link
Member Author

Kriechi commented May 30, 2016

Maybe I misunderstood you, but you sounded more enthusiastic about it in #233 (comment).

#228 sounds more like a 3.0 kinda thing. As a backward-compatible solution, putting priority information into RequestReceived is a good thing IMHO.

@Lukasa
Copy link
Member

Lukasa commented May 30, 2016

@Kriechi We can duplicate the priority information in there, but we can't move it there, because that would also be a breaking API change.

@Kriechi
Copy link
Member Author

Kriechi commented May 30, 2016

Yes, copying the values to the RequestReceived is fine.

@Lukasa
Copy link
Member

Lukasa commented Jun 1, 2016

So then, the question becomes which of the following APIs we want to go with:

  1. Add new data to RequestReceived that contains duplicate information from PriorityUpdated.
  2. Add a link from RequestReceived to a PriorityUpdated event if they were emitted simultaneously.
  3. Add support for emitting events as frozenset objects.

Doing 3 is a major version bump which we're considering anyway, so let's restrict ourselves to 1) and 2). Do you have a preference here?

@Kriechi
Copy link
Member Author

Kriechi commented Jun 1, 2016

1. is probably the simplest and requires only minimal changes / additions to the API.
2. sounds like a nice idea - not sure how efficient this can be done. Ideally all events with the same stream_id are linked to each other.

Unless you really dislike duplicating the values between two events, I would prefer 1.
But I'm also happy with 2.

@Lukasa Lukasa self-assigned this Jun 8, 2016
@Lukasa
Copy link
Member

Lukasa commented Jun 8, 2016

Ok, I'm prototyping a solution based on (2), because it has the handy property of being a good stepping stone to potentially implementing @rbtcollins' "frozenset" based idea for #228: that is, it'll get people used to the idea that they may want to check a set for other things before processing too far.

@Lukasa
Copy link
Member

Lukasa commented Jun 28, 2016

Resolved by #240.

@Lukasa Lukasa closed this as completed Jun 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants