IETF Logo Mail Archive

Re: [quicwg/base-drafts] Define AreAllPacketsLost() (#3290)

Alessandro Ghedini <notifications@github.com> Wed, 11 December 2019 13:13 UTC

Return-Path: <noreply@github.com>
X-Original-To: quic-issues@ietfa.amsl.com
Delivered-To: quic-issues@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 2FE50120119 for <quic-issues@ietfa.amsl.com>; Wed, 11 Dec 2019 05:13:17 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -6.596
X-Spam-Level:
X-Spam-Status: No, score=-6.596 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, HTML_IMAGE_ONLY_28=1.404, HTML_MESSAGE=0.001, MAILING_LIST_MULTI=-1, RCVD_IN_DNSWL_HI=-5, SPF_HELO_NONE=0.001, SPF_PASS=-0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=github.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id npTb8w4vqyUJ for <quic-issues@ietfa.amsl.com>; Wed, 11 Dec 2019 05:13:15 -0800 (PST)
Received: from out-18.smtp.github.com (out-18.smtp.github.com [192.30.252.201]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 5019D12011C for <quic-issues@ietf.org>; Wed, 11 Dec 2019 05:13:15 -0800 (PST)
Date: Wed, 11 Dec 2019 05:13:14 -0800
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=github.com; s=pf2014; t=1576069994; bh=r6lGbEIptj5kceLBCKEeatJ4G4LNR4rBG0S4gZwuPe0=; h=Date:From:Reply-To:To:Cc:In-Reply-To:References:Subject:List-ID: List-Archive:List-Post:List-Unsubscribe:From; b=VrQu7wNWbmUT5O47usWPD2HYHiYai62GQ7L8dtLWn0KjIlcQnq08QVBZDf2Jwp/Vc +gTEFBE0lmYgcLjDR0bo5XEOKFPL+5cjw9eX3MxNmFCelW01geMNOWrCoP2ICRBFRR Vj9U9GzmgFJottyFsqkVVSNZ5QcMtKkH/buRtm1Q=
From: Alessandro Ghedini <notifications@github.com>
Reply-To: quicwg/base-drafts <reply+AFTOJKY37XS6X434C4Y75UF37YO6VEVBNHHB7VVU2M@reply.github.com>
To: quicwg/base-drafts <base-drafts@noreply.github.com>
Cc: Subscribed <subscribed@noreply.github.com>
Message-ID: <quicwg/base-drafts/pull/3290/review/330528209@github.com>
In-Reply-To: <quicwg/base-drafts/pull/3290@github.com>
References: <quicwg/base-drafts/pull/3290@github.com>
Subject: Re: [quicwg/base-drafts] Define AreAllPacketsLost() (#3290)
Mime-Version: 1.0
Content-Type: multipart/alternative; boundary="--==_mimepart_5df0eb6a567e5_2bf73fc2be8cd960205155"; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: list
X-GitHub-Sender: ghedo
X-GitHub-Recipient: quic-issues
X-GitHub-Reason: subscribed
X-Auto-Response-Suppress: All
X-GitHub-Recipient-Address: quic-issues@ietf.org
Archived-At: <https://mailarchive.ietf.org/arch/msg/quic-issues/koS86yrUEkKWGem_yQ9CPc-DAnE>
X-BeenThere: quic-issues@ietf.org
X-Mailman-Version: 2.1.29
List-Id: Notification list for GitHub issues related to the QUIC WG <quic-issues.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/quic-issues/>
List-Post: <mailto:quic-issues@ietf.org>
List-Help: <mailto:quic-issues-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/quic-issues>, <mailto:quic-issues-request@ietf.org?subject=subscribe>
X-List-Received-Date: Wed, 11 Dec 2019 13:13:17 -0000

ghedo commented on this pull request.



> +     all_packets_lost =
+       lost_packets.length() == (lost_packets.last().packet_number -
+                                 lost_packets.first().packet_number + 1)

IMO the text or the comment should at the very least include the points that were raised in this disucssion, otherwise we'll just keep revisiting this over and over, and discussions on GitHub really won't help implementers once the spec is final. Also, I think the slight restructuring of the pseudo-code, where `InPersistentCongestion()` takes the whole list of lost packets is also more helpful to clarify what the information needed to make the decision is.

I can look into making another PR to do that, while keeping `AreAllPacketsLost()` undefined, if no one else wants to try.

-- 
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/3290#discussion_r356590369

v2.23.0 | Report a Bug | By Email