Subject: Re: [quicwg/base-drafts] Refactor DetectLostPackets (#2066)

From: ianswett <[email-masked]>
Date: Mon, 03 Dec 2018 18:21:18 -0800

ianswett commented on this pull request.

Thanks for working on this.  I think PR#1974 is close to landing, so I'd like to land that and then take another look at this.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#pullrequestreview-181067930

---

From: janaiyengar <[email-masked]>
Date: Wed, 05 Dec 2018 17:56:43 -0800

@janaiyengar pushed 1 commit.

71e06d391739652c1ed965873f530bb496f8aecd  lint

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066/files/02a164ee1e8e208e414896124c15c627f79dcea8..71e06d391739652c1ed965873f530bb496f8aecd

---

From: janaiyengar <[email-masked]>
Date: Wed, 05 Dec 2018 18:01:00 -0800

cdaef0e2036dc9da2c972a9ec0d2c795559c3206  cleanup and consistent parens

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066/files/71e06d391739652c1ed965873f530bb496f8aecd..cdaef0e2036dc9da2c972a9ec0d2c795559c3206

---

From: MikkelFJ <[email-masked]>
Date: Thu, 06 Dec 2018 00:09:37 -0800

How about gaps in PN sequence (optimistic ACK defense)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#issuecomment-444784125

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 06:44:29 -0800

ianswett approved this pull request.

This looks good, but I think you should move the where the timers in the past comment lives.

>    if (!lost_packets.empty()):
     OnPacketsLost(lost_packets)
 ~~~
 
+This algorithm results in loss_time being set based on the earliest packet that
+is still in flight.  Timers set based on a loss_time that has already passed

I think this comment about timers with timeouts that have already passed is more widely applicable, so maybe move it closer to SetLossDetectionTimer()?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#pullrequestreview-182271374

---

From: Martin Thomson <[email-masked]>
Date: Thu, 06 Dec 2018 13:55:04 -0800

Thanks for cleaning this up Jana.  I wasn't sure if the timers comment was scoped specifically to this particular instance, so I didn't make it a more general statement.  @ianswett, do you think that you could move that for me?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#issuecomment-445043864

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 14:54:58 -0800

@ianswett pushed 1 commit.

e50cf51845fdfb8357e974bdc2ad4134d3829663  Update draft-ietf-quic-recovery.md

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066/files/cdaef0e2036dc9da2c972a9ec0d2c795559c3206..e50cf51845fdfb8357e974bdc2ad4134d3829663

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 14:55:50 -0800

Thanks @martinthomson, I updated to what I believe is the right location.  Feel free to update further.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#issuecomment-445060388

---

From: ianswett <[email-masked]>
Date: Thu, 06 Dec 2018 14:55:59 -0800

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#pullrequestreview-182487668

---

From: janaiyengar <[email-masked]>
Date: Thu, 06 Dec 2018 23:54:13 +0000 (UTC)

@mikkelfj: the loss recovery algorithm does not handle arbitrary gaps in sequence space. This can be handled with some complexity, which is probably worth writing a sentence or two about. Mind filing an issue?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#issuecomment-445073303

---

From: janaiyengar <[email-masked]>
Date: Thu, 06 Dec 2018 15:54:22 -0800

janaiyengar approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#pullrequestreview-182502868

---

From: janaiyengar <[email-masked]>
Date: Thu, 06 Dec 2018 15:54:29 -0800

Merged #2066 into master.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/2066#event-2011259830