Subject: [quicwg/base-drafts] Editorial fixes for recovery (#4213)

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 13:58:45 -0700

Addresses #4196.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * Magnus comments

-- File Changes --

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

-- Patch Links --

https://github.com/quicwg/base-drafts/pull/4213.patch
https://github.com/quicwg/base-drafts/pull/4213.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/4213

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 14:26:58 -0700

@janaiyengar pushed 1 commit.

a3f807b922415db5a45a87f6a07acf4dd52f24aa  lint

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4213/files/4700b16db2925e40c014a1bfe5aa9a3d4db8c6df..a3f807b922415db5a45a87f6a07acf4dd52f24aa

---

From: ianswett <[email-masked]>
Date: Wed, 14 Oct 2020 15:14:41 -0700

@ianswett commented on this pull request.

Mostly LG, but this PR makes it clear to me that the RTT estimator initialization text doesn't flow that well.

> @@ -403,18 +403,25 @@ until the handshake is confirmed.
 
 smoothed_rtt and rttvar are computed as follows, similar to {{?RFC6298}}.
 
-When there are no samples for a network path, and on the first RTT sample for
-the network path:
+An endpoint needs to bootstrap the RTT estimator during connection establishment

I'm not sure if initialize is right, but bootstrap is not a commonly used word.

```suggestion
An endpoint needs to initialize the RTT estimator during connection establishment
```

 ~~~
 smoothed_rtt = rtt_sample
 rttvar = rtt_sample / 2
 ~~~
 
-Before any RTT samples are available, the initial RTT is used as rtt_sample.  On
-the first RTT sample for the network path, that sample is used as rtt_sample.
-This ensures that the first measurement erases the history of any persisted or
-default values.
+Before any RTT samples are available for a new path or when the estimator is

How about moving these two paragraph above the paragraph starting with "When there are no RTT samples..." and reworking the text a bit as a result?

-- 
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/4213#pullrequestreview-508804974

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:12:19 -0700

@janaiyengar commented on this pull request.

 ~~~

I actually did exactly that, and realized that it's awkward to use `rtt_sample` in that structure, since it's not yet been used. But yeah, this still isn't great. I'll work on it a bit.

-- 
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/4213#discussion_r505107162

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:12:24 -0700

d6fb59baa5ad2b72854fc9f1ccb6619081a0099c  Apply suggestions from code review

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

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:22:46 -0700

609e3521d5372a7fd4418e9e1c2da401faf7ca26  rework text

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4213/files/d6fb59baa5ad2b72854fc9f1ccb6619081a0099c..609e3521d5372a7fd4418e9e1c2da401faf7ca26

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:23:16 -0700

~~~

Ok, done.

-- 
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/4213#discussion_r505109890

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:29:20 -0700

09b3e0ad27b56aa0f4c68dd14e1bd50b6e22a037  lint

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4213/files/609e3521d5372a7fd4418e9e1c2da401faf7ca26..09b3e0ad27b56aa0f4c68dd14e1bd50b6e22a037

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:43:37 -0700

fb71e7ad639eba345fb98d95c0f0d1680e7f7de4  Fix structural and reference issues

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/4213/files/09b3e0ad27b56aa0f4c68dd14e1bd50b6e22a037..fb71e7ad639eba345fb98d95c0f0d1680e7f7de4

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 14 Oct 2020 18:45:57 -0700

I've relented and just written things out a bit more explicitly. I think this is much clearer now, and also fixes the structural issues that @gloinul noted in #4192.

-- 
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/4213#pullrequestreview-508881194

---

From: ianswett <[email-masked]>
Date: Wed, 14 Oct 2020 18:49:23 -0700

A few more suggestions, but this is looking good.

 ~~~
 
-Before any RTT samples are available, the initial RTT is used as rtt_sample.  On
-the first RTT sample for the network path, that sample is used as rtt_sample.
-This ensures that the first measurement erases the history of any persisted or
-default values.
+In the computation above, rtt_sample is initialized to the initial RTT value,

I'd prefer to remove this paragraph.

> +An endpoint needs to initialize the RTT estimator during connection establishment
+and when the estimator is reset during connection migration (Section 9.4 of
+{{QUIC-TRANSPORT}}).

```suggestion
An endpoint initializes the RTT estimator during connection establishment
and when the estimator is reset during connection migration (Section 9.4 of
{{QUIC-TRANSPORT}}).
```

> +Before any RTT samples are available for a new path or when the estimator is
+reset, the estimator is initialized using an initial RTT value of 333ms; see

```suggestion
Before any RTT samples are available for a new path, the estimator is
initialized using an initial RTT value of 333ms; see
```

-- 
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/4213#pullrequestreview-508881074

---

From: ianswett <[email-masked]>
Date: Wed, 14 Oct 2020 18:52:19 -0700

@ianswett approved this pull request.

That's explicit, but clear.

-- 
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/4213#pullrequestreview-508883115

---

From: Martin Thomson <[email-masked]>
Date: Wed, 14 Oct 2020 19:54:17 -0700

@martinthomson approved this pull request.

When in doubt, spell it out.

+{{QUIC-TRANSPORT}}).

```suggestion
and when the estimator is reset during connection migration; see Section 9.4 of
{{QUIC-TRANSPORT}}.
```

> +reset, the estimator is bootstrapped using the initial RTT value for
+rtt_sample.

```suggestion
reset, the estimator uses the initial RTT value for rtt_sample.
```

-- 
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/4213#pullrequestreview-508839331

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 15 Oct 2020 05:19:14 -0700

@gloinul approved this pull request.

Looks good and appear to resolve the uncertainties I had. 

-- 
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/4213#pullrequestreview-509315573

---

From: ianswett <[email-masked]>
Date: Thu, 15 Oct 2020 06:45:47 -0700

One nit and one substantive comment about not repeating the 333ms value.

-When there are no samples for a network path, and on the first RTT sample for
-the network path:
+An endpoint needs to initialize the RTT estimator during connection

```suggestion
An endpoint needs to initialize the RTT estimator during connection
```
```suggestion
An endpoint initializes the RTT estimator during connection
```

> +(Section 9.4 of {{QUIC-TRANSPORT}}). Before any RTT samples are available for a
+new path or when the estimator is reset, the estimator is initialized using an
+initial RTT value of 333ms; see {{pto-handshake}}.

This is slightly different from what we say in handshakes and new paths(pto-handshake).  I'd suggest referencing that rather than re-specifying 333ms here.
```suggestion
(Section 9.4 of {{QUIC-TRANSPORT}}). Before any RTT samples are available for a
new path or when the estimator is reset, the estimator is initialized using the
initial RTT; see {{pto-handshake}}.
```

FYI, existing text is:
   Resumed connections over the same network MAY use the previous
   connection's final smoothed RTT value as the resumed connection's
   initial RTT.  When no previous RTT is available, the initial RTT
   SHOULD be set to 333ms, resulting in a 1 second initial timeout, as
   recommended in [RFC6298].

-- 
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/4213#pullrequestreview-509383324

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 15 Oct 2020 12:37:15 -0700

Fair enough.

-- 
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/4213#discussion_r505792032

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 15 Oct 2020 12:37:27 -0700

f9869890fdc959aaf7d31766fb89a5a90a040b72  Apply suggestions from code review

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

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 15 Oct 2020 12:41:38 -0700

e1b3e900c380a51a18975cc347940acc1207428a  reference

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

---

From: ianswett <[email-masked]>
Date: Thu, 15 Oct 2020 12:46:26 -0700

-- 
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/4213#pullrequestreview-509715627

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 15 Oct 2020 12:56:53 -0700

Merged #4213 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/4213#event-3883332121

---

From: Martin Thomson <[email-masked]>
Date: Thu, 15 Oct 2020 17:46:36 -0700

Oops, I had a few comments left over.

-- 
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/4213#pullrequestreview-509941497