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

Adaptive thresholds are useful #3572

Merged
merged 9 commits into from
May 13, 2020
Merged

Adaptive thresholds are useful #3572

merged 9 commits into from
May 13, 2020

Conversation

ianswett
Copy link
Contributor

@ianswett ianswett commented Apr 8, 2020

Fixes #3571 by tweaking the text, but doesn't add a new SHOULD.

@maolson-msft
Copy link

Since RFC4653 doesn't seem like it would map to QUIC as readily as the stuff in the RACK draft about reorder-threshold tuning (it seems to be all about extending the Limited Transmit algorithm), maybe RACK is a better reference?

https://tools.ietf.org/pdf/draft-ietf-tcpm-rack-08.pdf
c.f. "Update RACK reordering window"

@ianswett
Copy link
Contributor Author

ianswett commented Apr 8, 2020

I would prefer to reference RACK, but it's not an RFC yet and I'm not sure it will be prior to QUIC shipping. What's the best way to handle that?

@maolson-msft
Copy link

The QUIC recovery spec already references the RACK draft in multiple places.

@ianswett
Copy link
Contributor Author

ianswett commented Apr 9, 2020

Fair point, I replaced the old reference with the RACK reference. PTAL.

@ianswett ianswett added the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 9, 2020
@maolson-msft
Copy link

LG!

@ianswett ianswett changed the title SHOULD use adaptive thresholds Adaptive thresholds are useful Apr 14, 2020
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
Copy link
Contributor

@janaiyengar janaiyengar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slight rewording

draft-ietf-quic-recovery.md Outdated Show resolved Hide resolved
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
@maolson-msft
Copy link

I don't see any conversation in this PR about it, but I see a commit titled simply "non-normative" reverting the reference from RACK back to NCR. It's not a big deal, but I'm just curious about the reasoning.

@ianswett
Copy link
Contributor Author

There's conversation on the issue(#3571 (comment)) and Jana was concerned about adding another reference to RACK as well as recommending something that we don't have a detailed algorithm(and pseudocode) for. In general, anything we have a SHOULD for has more text(and typically pseudocode) than this, so I accepted that point and adjusted the PR. For example, the pseudocode doesn't detail how to detect spurious losses, since it's expected that could be fairly complex and implementation dependent.

@ianswett ianswett removed the design An issue that affects the design of the protocol; resolution requires consensus. label Apr 23, 2020
@maolson-msft
Copy link

I see, thanks for pointing me to that. But Jana's ask seems to me to be all about SHOULD versus MAY, and orthogonal to referencing NCR vs RACK; if it was a SHOULD, then you'd still have the same incompletely-specified-algorithm problem with NCR. It seems to me that now that SHOULD is removed, Jana would agree that RACK is a better reference since it maps more cleanly to QUIC.

@maolson-msft
Copy link

Actually, I'm not so confident that RACK maps all that cleanly either, and Jana made the point that RACK's ReoWnd adjustment algorithm hasn't been closely examined, so I'm going to bow out here- I'm happy with either NCR or RACK.

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. editorial An issue that does not affect the design of the protocol; does not require consensus. and removed design An issue that affects the design of the protocol; resolution requires consensus. labels Apr 28, 2020
@ianswett ianswett merged commit beed8a8 into master May 13, 2020
@ianswett ianswett deleted the ianswett-adaptive-loss branch May 13, 2020 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-recovery editorial An issue that does not affect the design of the protocol; does not require consensus.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SHOULD implement adaptive packet threshold loss detection
5 participants