Subject: [quicwg/base-drafts] editorial: This sentence was hard to parse for me (recovery) (#3383)

From: mirjak <[email-masked]>
Date: Wed, 22 Jan 2020 08:39:30 -0800

So here is an alternative proposal. However I also wonder why this paragraph is a MUST requirement while the previous statement (avoiding multiple samples of the same packet) is only a SHOULD? I would see it rather the other way around or both MUSTs.
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * editorial: This sentence was hard to parse for me (recovery)

-- File Changes --

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

-- Patch Links --

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

---

From: Dmitri Tikhonov <[email-masked]>
Date: Wed, 22 Jan 2020 09:05:30 -0800

dtikhonov commented on this pull request.

> @@ -272,9 +272,10 @@ SHOULD NOT be used to update RTT estimates if it does not newly acknowledge the
 largest acknowledged packet.
 
 An RTT sample MUST NOT be generated on receiving an ACK frame that does not
-newly acknowledge at least one ack-eliciting packet.  A peer does not send an
-ACK frame on receiving only non-ack-eliciting packets, so an ACK frame that is
-subsequently sent can include an arbitrarily large Ack Delay field.  Ignoring
+newly acknowledge at least one ack-eliciting packet.  A peer does usally not

s/usally/usually/

Also, I'd place "usually" before does:

> A peer usually does not send [...]

-- 
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/3383#pullrequestreview-346753717

---

From: Mike Bishop <[email-masked]>
Date: Wed, 22 Jan 2020 09:44:34 -0800

MikeBishop commented on this pull request.

"does not usually" would also be fine.  But not in the middle.

 An RTT sample MUST NOT be generated on receiving an ACK frame that does not
-newly acknowledge at least one ack-eliciting packet.  A peer does not send an
-ACK frame on receiving only non-ack-eliciting packets, so an ACK frame that is
-subsequently sent can include an arbitrarily large Ack Delay field.  Ignoring
+newly acknowledge at least one ack-eliciting packet.  A peer does usally not
+send an ACK frame when only non-ack-eliciting packets are received. Therefore
+an ACK frame that contains only acknowledge for non-ack-eliciting packets is

```suggestion
an ACK frame that only contains acknowledgements for non-ack-eliciting packets
```
May require a rewrap

-- 
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/3383#pullrequestreview-346779206

---

From: mirjak <[email-masked]>
Date: Wed, 22 Jan 2020 14:16:47 -0800

@mirjak pushed 1 commit.

d3cc03174ba5f0a7d7bbcc451d08c920a0fdf9d1  English hard be...

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/3383/files/8de90213184a323618a32502904f37673640ca5d..d3cc03174ba5f0a7d7bbcc451d08c920a0fdf9d1

---

From: mirjak <[email-masked]>
Date: Wed, 22 Jan 2020 14:17:04 -0800

mirjak commented on this pull request.

thanks. 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/3383#discussion_r369834923

---

From: mirjak <[email-masked]>
Date: Wed, 22 Jan 2020 14:17:12 -0800

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/3383#discussion_r369834979

---

From: Martin Thomson <[email-masked]>
Date: Wed, 22 Jan 2020 14:37:03 -0800

martinthomson approved this pull request.

This is a great improvement, thanks.

One additional suggestion (for existing text) below.

 An RTT sample MUST NOT be generated on receiving an ACK frame that does not
-newly acknowledge at least one ack-eliciting packet.  A peer does not send an
-ACK frame on receiving only non-ack-eliciting packets, so an ACK frame that is
-subsequently sent can include an arbitrarily large Ack Delay field.  Ignoring
+newly acknowledge at least one ack-eliciting packet. A peer usually does not
+send an ACK frame when only non-ack-eliciting packets are received. Therefore
+an ACK frame that only contains acknowledgements for non-ack-eliciting packets
+can subsequently include an arbitrarily large Ack Delay value.  Ignoring

```suggestion
could include an arbitrarily large Ack Delay value.  Ignoring
```

-- 
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/3383#pullrequestreview-346952994

---

From: Jana Iyengar <[email-masked]>
Date: Wed, 22 Jan 2020 18:39:18 -0800

janaiyengar commented on this pull request.

```suggestion
an ACK frame that contains acknowledgements for only non-ack-eliciting packets
```

-- 
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/3383#pullrequestreview-347031038

---

From: mirjak <[email-masked]>
Date: Thu, 23 Jan 2020 01:24:11 -0800

@mirjak pushed 1 commit.

f8162becbde33a2e11f43f303b511c3dad949ade  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/3383/files/d3cc03174ba5f0a7d7bbcc451d08c920a0fdf9d1..f8162becbde33a2e11f43f303b511c3dad949ade

---

From: mirjak <[email-masked]>
Date: Thu, 23 Jan 2020 01:24:30 -0800

@mirjak pushed 1 commit.

81f10dc8a143f6da754daab8690256c0597a6d07  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/3383/files/f8162becbde33a2e11f43f303b511c3dad949ade..81f10dc8a143f6da754daab8690256c0597a6d07

---

From: Mike Bishop <[email-masked]>
Date: Thu, 23 Jan 2020 07:57:46 -0800

Technically, it could still acknowledge other packets as well -- the key is which packets are *newly* acknowledged, isn't it?

-- 
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/3383#discussion_r370204329

---

From: Jana Iyengar <[email-masked]>
Date: Thu, 23 Jan 2020 14:25:24 -0800

Merged #3383 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/3383#event-2975501797