-
Notifications
You must be signed in to change notification settings - Fork 204
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
Clarify time threshold vs crypto timer #2620
Conversation
Adds some explanation about why it's better to arm/fire the time threshold alarm before the crypto retransmission timeout. Hopefully clarifies comments from #2565
Thanks for the explanation. It's much clearer. |
Index DetectLostPackets by [pn_space]
Thanks for catching the pseudocode error in DetectLostPackets. PTAL. |
lgtm. thank you! |
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.
A couple of comments.
Co-Authored-By: ianswett <ianswett@users.noreply.github.com>
draft-ietf-quic-recovery.md
Outdated
retransmission timeout and be less likely to spuriously retransmit data. | ||
The Initial and Handshake packet number spaces typically have a small number | ||
of packets in them, so time threshold loss detection will typically declare | ||
packets lost before packet threshold. |
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.
I think what you want to say is that time-threshold is more likely to be used for loss detection than packet-threshold, since there just might not be enough packets. So how about just this, given that you've just talked about time-threshold: "The Initial and Handshake packet number spaces will typically carry a small number of packets, so losses are less likely to be detected using packet-threshold detection."
That said, I still am not sure that it's worth mentioning the packet-threshold here at all. But my opinion is not strong, so your call.
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.
Thanks, suggestion taken. I think why you'd arm the loss-detection timer is non-obvious, so I thought more explanatory text was better than less, and I think the small number of packets argument is relevant.
Jana's suggestion
Adds some explanation about why it's better to arm/fire the time threshold alarm before the crypto retransmission timeout.
Hopefully clarifies comments from #2565