Subject: [quicwg/base-drafts] Add 'time' to latest_in_flight_lost (#4434)

From: ianswett <[email-masked]>
Date: Thu, 10 Dec 2020 05:07:11 -0800

I&#39;m not sure this is the best name, but I think adding &#39;time&#39; to the variable makes this easier to read, particularly since the variable is initialized to 0, which is not really a time.

Follow-up to #4426
You can view, comment on, or merge this pull request online at:

  https://github.com/quicwg/base-drafts/pull/4434

-- Commit Summary --

  * Add &#39;time&#39; to latest_in_flight_lost

-- File Changes --

    M draft-ietf-quic-recovery.md (10)

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4434.patch
https://github.com/quicwg/base-drafts/pull/4434.diff

-- 
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/4434

---

From: Lars Eggert <[email-masked]>
Date: Thu, 10 Dec 2020 05:08:58 -0800

time_of_latest_in_flight_loss?

-- 
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/4434#issuecomment-742510542

---

From: ianswett <[email-masked]>
Date: Thu, 10 Dec 2020 05:10:06 -0800

@ianswett pushed 1 commit.

a8acd4f6fbd38e8a9a6c8651cc36759e39be129f  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/4434/files/e31cea24525fa59ec6a83584751c298d204613bb..a8acd4f6fbd38e8a9a6c8651cc36759e39be129f

---

From: ianswett <[email-masked]>
Date: Thu, 10 Dec 2020 05:18:25 -0800

@larseggert SGTM, PTAL

-- 
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/4434#issuecomment-742515423

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 10 Dec 2020 11:30:39 -0800

@janaiyengar approved this pull request.

This isn't the time of the loss, it's the time that this packet was sent. Which is why I left `time` out of the name entirely. Maybe `sent_time_of_latest_in_flight_loss` if you want to be really accurate?

Alternatively, it's a local variable. You could do something like:
```
// t records the send time of the latest inflight packet
// that was marked as lost.
t = 0
```

-- 
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/4434#pullrequestreview-549533367

---

From: ianswett <[email-masked]>
Date: Thu, 10 Dec 2020 12:03:11 -0800

How about sent_time_of_latest_loss?

Technically, packets that aren't in-flight are never declared lost so I don't think in_flight is necessary in the variable name.

-- 
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/4434#issuecomment-742764038

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 10 Dec 2020 18:54:28 -0800

@ianswett - that works!

-- 
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/4434#issuecomment-742932127

---

From: Martin Thomson <[email-masked]>
Date: Thu, 10 Dec 2020 19:12:12 -0800

or `sent_time_of_last_loss` if you need two more bytes back

-- 
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/4434#issuecomment-742937382

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 10 Dec 2020 20:48:49 -0800

Not for this PR, but this does point out a slight ambiguity in the description of persistent congestion, which has as its first condition: `all packets, across all packet number spaces, sent between the send times of these two packets are declared lost`

If non-ack-eliciting packets are never declared lost, that makes this text a bit odd to square away.

-- 
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/4434#issuecomment-742966423

---

From: Martin Thomson <[email-masked]>
Date: Thu, 10 Dec 2020 22:04:23 -0800

I don't think that's a problem.  When you declare the trailing ack-eliciting packet lost, all packets sent in the same packet number space before the lost packet will also be declared lost (unless they were acknowledged, of course).

The only risk is that there might be packets (both ack-eliciting and non-ack-eliciting) from a different packet number space that aren't declared lost.  Mostly, this is because that space might not have newer packets are are acknowledged.  I think that we can tolerate that little bit of infidelity.

-- 
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/4434#issuecomment-742991631

---

From: ianswett <[email-masked]>
Date: Fri, 11 Dec 2020 14:31:32 -0800

1d6e7068af4438bee467d8a88190a05c6d4b5d10  MT's suggestion

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4434/files/a8acd4f6fbd38e8a9a6c8651cc36759e39be129f..1d6e7068af4438bee467d8a88190a05c6d4b5d10

---

From: ianswett <[email-masked]>
Date: Fri, 11 Dec 2020 14:32:34 -0800

I took @martinthomson last suggestion of sent_time_of_last_loss.  Feel free to merge whenever(sooner the better I expect).

-- 
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/4434#issuecomment-743461571

---

From: Jana Iyengar <[email-masked]>
Date: Fri, 11 Dec 2020 15:32:46 -0800

Merged #4434 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/4434#event-4102850466

---

From: Jana Iyengar <[email-masked]>
Date: Sun, 13 Dec 2020 00:20:47 -0800

@martinthomson : Technically, non-ack-eliciting packets are never "declared lost". I've proposed a fix in #4437.

-- 
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/4434#issuecomment-743972537