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.

Sign up for GitHub

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

Jump to bottom

Adjusting latest RTT by ack delay #2062

Closed
wants to merge 1 commit into from
Closed

Adjusting latest RTT by ack delay #2062

wants to merge 1 commit into from

Conversation

ianswett
Copy link
Contributor

Fixes #2060

kazuho
kazuho approved these changes Nov 28, 2018
View reviewed changes
Copy link
Member

@kazuho kazuho left a comment

Choose a reason for hiding this comment

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

❤️

@martinthomson martinthomson added design An issue that affects the design of the protocol; resolution requires consensus. -recovery labels Nov 28, 2018
@janaiyengar janaiyengar mentioned this pull request Dec 5, 2018
Closed
@janaiyengar
Copy link
Contributor

I still think we should ignore this RTT sample instead.

@ianswett
Copy link
Contributor Author

ianswett commented Dec 7, 2018

I'm increasingly leaning towards abandoning this PR and landing #2099

I'm scared of ignoring RTT samples. In theory, it should work, but I'm worried about edge cases that are difficult to reason about.

@janaiyengar
Copy link
Contributor

Fair enough. I think the current spec text is still safe, so I'd be fine with it.

@ianswett ianswett closed this Dec 7, 2018
@MikeBishop MikeBishop deleted the ianswett-latest-ack-delay branch February 6, 2019 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kazuho kazuho kazuho approved these changes

@janaiyengar janaiyengar Awaiting requested review from janaiyengar

Assignees
No one assigned
Labels
-recovery design An issue that affects the design of the protocol; resolution requires consensus.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ianswett @janaiyengar @kazuho @martinthomson