-
Notifications
You must be signed in to change notification settings - Fork 23
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
Peeps First Attempt #494
Peeps First Attempt #494
Conversation
How does one close a peep? Currently, in our implementation, we rely in combination of delivery preference and object status for that. |
I think a Stream FIN would be sufficient. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good overall, some comments/questions
How am I supposed to know when to send a FIN? |
The publisher sends a FIN when the Peep is done. The relay knows a Peep is complete when it receives the FIN from upstream. |
Is there ever a case where a publisher needs to close a stream and ensure all data on it is delivered, but does not know that the peep is done, or knows it is not done. For example - in response to an unsubscribe or graceful closure. |
How is this represented in the cache? Are FINs guaranteed to be always at the same position? One scenario where the sender might FIN a peep earlier is if the next object in the peep is past subscription end, though I'm not sure how we'd hit this in practice. |
Good point, I've added an end of Peep object. |
OK, I believe I've fully addressed all comments except for those I've left unresolved. @suhasHere @ianswett If you are satisfied with my response, please resolve the comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thansk @martinduke for the new set of edits. I just had few nits/clarification points. PLease let me know your thoughts.
I am liking the general direction.
I have made all changes I know to do. I am awaiting @afrind making a concrete proposal on the question of whether or not two Peeps can have the same priority, and the wire image implications downstream from that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good overall, some detailed suggestions.
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Why have Peep ID be a tiebreaker if we also have priority? Just let the streams have the same priority! |
Co-authored-by: ianswett <ianswett@users.noreply.github.com>
Removing TODO around MOQ Streaming Format registration
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually agree with the big picture here but bunch of details I think need to be sorted out before we merge.
higher Object Id, regardless of the specified Group Order. If the group | ||
contains more than one Peep and the priority varies between these Peeps, | ||
higher priority Peeps are sent before lower priority Peeps. If the specified | ||
priority of two Peeps in a Group are equal, the lower Peep ID has priority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an application developer, I think I want the opposite of this. If I want different priorities, I will set the priority. But if I want to fair share across two peeps I would set the priorities to be the same. If we peep-ID also influences the priority, there is no way to do this because the peed-id can not be the same. My suggestion would be to remove this stuff about peep-id changing any priorities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is preserving what is in the current draft for stream-per-object - there was never a way to fair share objects in this mode, because Object ID was the tiebreaker.
Adding Peeps opens the question of if we want to be able to fair share peeps. In the interim I argued for a use case, but after consulting with the originators of said use case, I decided that a strict ordering is probably what they want. I propose we advance this PR keeping as close to the existing behavior (Peep ID tiebreakers), and revisit fair-sharing them separately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems just less flexible than the alternative I can't see why we would do that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cullen makes the point that the current algorithm works, if one views objects as the thing that holds priority. Within the group, the most important objects (peeps) will go first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@afrind makes the point that it's hard to implement this because you'd have to change stream priorities mid-stream as the object IDs change, which QUIC APIs are generally bad it.
I'm not deeply concerned either way, as priorities are somewhat advisories. I'd just like these two to come to some agreement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Individual Comments
higher Object Id, regardless of the specified Group Order. If the group | ||
contains more than one Peep and the priority varies between these Peeps, | ||
higher priority Peeps are sent before lower priority Peeps. If the specified | ||
priority of two Peeps in a Group are equal, the lower Peep ID has priority. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is preserving what is in the current draft for stream-per-object - there was never a way to fair share objects in this mode, because Object ID was the tiebreaker.
Adding Peeps opens the question of if we want to be able to fair share peeps. In the interim I argued for a use case, but after consulting with the originators of said use case, I decided that a strict ordering is probably what they want. I propose we advance this PR keeping as close to the existing behavior (Peep ID tiebreakers), and revisit fair-sharing them separately.
Block and Stream work for me. I was going to say that I prefer "QUIC Stream", but given this might be over WebTransport, technically it'd be a WebTransport stream, so defining the term "Stream" at the top of the document should make that clear. |
One more name suggestion : SubGroup |
Merging with the understanding that we'll continue to discuss how prioritization should work. |
I have foregone any premature optimization of the encodings here.
Asking @janaiyengar to take a first look.