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

Add clarification for is_coalesced to packet_sent and packet_received #370

Open
rmarx opened this issue Jan 19, 2024 · 3 comments · May be fixed by #403
Open

Add clarification for is_coalesced to packet_sent and packet_received #370

rmarx opened this issue Jan 19, 2024 · 3 comments · May be fixed by #403
Assignees

Comments

@rmarx
Copy link
Contributor

rmarx commented Jan 19, 2024

As reported by @hlandau on the mailing list:

quic:packet_sent:

The meaning of is_coalesced should be clarified. I assume this is
set to true unless a packet is both the first and last packet in a
datagram. However that is not necessarily obvious when a packet is
being generated; another option is to define this as being true for
every packet but the first packet in a datagram.

quic:packet_received:

See comment on is_coalesced above

@rmarx rmarx self-assigned this Jan 19, 2024
@rmarx
Copy link
Contributor Author

rmarx commented Mar 1, 2024

So, I clarified this in #403, but I'm not longer fully sure the is_coalesced approach is the best.

Maybe @hlandau could take another look now that the intent is clearer and indicate which approach you prefer? :)

@hlandau
Copy link

hlandau commented Mar 6, 2024

@rmarx My reading here:

However, in cases where implementations cannot track datagrams in this way, the
is_coalesced field can be used to indicate that a packet was (intended to be)
coalesced. Implementations MAY use both methods concurrently.

is that is_coalesced is basically non-preferred and only for use if an implementation cannot use datagram_id. That seems like a good approach.

"intended to be" needs clarifying... this implies a packet could be intended to be coalesced but isn't actually. So to have that you actually have to introduce a concept of "coalescable" rather than "was coalesced", which feels like it opens a bit of a can of worms where either we have to define that, or an implementation basically gets to define some arbitrary set of conditions which is "coalescing desired". But that then kind of feels uselessly vague.

I almost feel like we should just drop is_coalesced unless we can identify a concrete use case for it. It seems hard to lock down a proper meaning for it and the utility (unless I am missing something) seems minimal given the minimal implementation costs of the preferred datagram_id approach.

@LPardue
Copy link
Member

LPardue commented Mar 6, 2024

Yeah I'm in Hugo, per my comment on PR #403 , datagram_id seems yseful enough without the is_coaleaced field and avoids the can o' worms

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 a pull request may close this issue.

3 participants