From reviews-return-57767-apmail-mesos-reviews-archive=mesos.apache.org@mesos.apache.org Sun Mar 19 04:22:02 2017 Return-Path: X-Original-To: apmail-mesos-reviews-archive@minotaur.apache.org Delivered-To: apmail-mesos-reviews-archive@minotaur.apache.org Received: from mail.apache.org (hermes.apache.org [140.211.11.3]) by minotaur.apache.org (Postfix) with SMTP id 4B06A19832 for ; Sun, 19 Mar 2017 04:22:02 +0000 (UTC) Received: (qmail 88576 invoked by uid 500); 19 Mar 2017 04:22:02 -0000 Delivered-To: apmail-mesos-reviews-archive@mesos.apache.org Received: (qmail 88544 invoked by uid 500); 19 Mar 2017 04:22:02 -0000 Mailing-List: contact reviews-help@mesos.apache.org; run by ezmlm Precedence: bulk List-Help: List-Unsubscribe: List-Post: List-Id: Reply-To: reviews@mesos.apache.org Delivered-To: mailing list reviews@mesos.apache.org Received: (qmail 88533 invoked by uid 99); 19 Mar 2017 04:22:01 -0000 Received: from pnap-us-west-generic-nat.apache.org (HELO spamd4-us-west.apache.org) (209.188.14.142) by apache.org (qpsmtpd/0.29) with ESMTP; Sun, 19 Mar 2017 04:22:01 +0000 Received: from localhost (localhost [127.0.0.1]) by spamd4-us-west.apache.org (ASF Mail Server at spamd4-us-west.apache.org) with ESMTP id 3D8BEC05B0; Sun, 19 Mar 2017 04:22:01 +0000 (UTC) X-Virus-Scanned: Debian amavisd-new at spamd4-us-west.apache.org X-Spam-Flag: NO X-Spam-Score: 4.451 X-Spam-Level: **** X-Spam-Status: No, score=4.451 tagged_above=-999 required=6.31 tests=[DKIM_ADSP_CUSTOM_MED=0.001, HEADER_FROM_DIFFERENT_DOMAINS=0.001, HTML_MESSAGE=2, KAM_LAZY_DOMAIN_SECURITY=1, KAM_LOTSOFHASH=0.25, NML_ADSP_CUSTOM_MED=1.2, RP_MATCHES_RCVD=-0.001] autolearn=disabled Received: from mx1-lw-us.apache.org ([10.40.0.8]) by localhost (spamd4-us-west.apache.org [10.40.0.11]) (amavisd-new, port 10024) with ESMTP id IzqsJj6MNH5v; Sun, 19 Mar 2017 04:21:59 +0000 (UTC) Received: from mailrelay1-us-west.apache.org (mailrelay1-us-west.apache.org [209.188.14.139]) by mx1-lw-us.apache.org (ASF Mail Server at mx1-lw-us.apache.org) with ESMTP id 88DE65FB4E; Sun, 19 Mar 2017 04:21:59 +0000 (UTC) Received: from reviews.apache.org (unknown [10.41.0.12]) by mailrelay1-us-west.apache.org (ASF Mail Server at mailrelay1-us-west.apache.org) with ESMTP id 00051E00C7; Sun, 19 Mar 2017 04:21:58 +0000 (UTC) Received: from reviews-vm2.apache.org (localhost [IPv6:::1]) by reviews.apache.org (ASF Mail Server at reviews-vm2.apache.org) with ESMTP id C896CC4047A; Sun, 19 Mar 2017 04:21:58 +0000 (UTC) Content-Type: multipart/alternative; boundary="===============2318816429826766581==" MIME-Version: 1.0 Subject: Review Request 57752: Fixed crash with tasks that use very small resource values. From: Neil Conway To: Benjamin Bannier , Joris Van Remoortere Cc: Neil Conway , mesos Date: Sun, 19 Mar 2017 04:21:58 -0000 Message-ID: <20170319042158.41091.37544@reviews-vm2.apache.org> X-ReviewBoard-URL: https://reviews.apache.org/ Auto-Submitted: auto-generated Sender: Neil Conway X-ReviewGroup: mesos X-Auto-Response-Suppress: DR, RN, OOF, AutoReply X-ReviewRequest-URL: https://reviews.apache.org/r/57752/ X-Sender: Neil Conway Reply-To: Neil Conway X-ReviewRequest-Repository: mesos --===============2318816429826766581== MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57752/ ----------------------------------------------------------- Review request for mesos, Benjamin Bannier and Joris Van Remoortere. Bugs: MESOS-7197 https://issues.apache.org/jira/browse/MESOS-7197 Repository: mesos Description ------- When parsing resources, inputs such as "cpus:0" are considered "empty" and are omitted from the `Resources` object. However, a very small resource value like "cpus:0.00001" was considered non-empty, despite the fact that the numerical behavior for resources means that these two values compare as equal. This inconsistency resulted in a `CHECK` failure in the sorter. This commit fixes the resource parsing logic to treat such very small resource values as empty resources, which resolves the inconsistency above. The resulting behavior might be a bit surprising to users (since the task will launch with zero resources instead of a very small resource value), but improving this is left as future work (MESOS-1807). Diffs ----- src/common/resources.cpp b64fd76d1d3d8179bae2ac5b335bdf4ad980c9a7 src/tests/master_tests.cpp 5fdb9493c9aed31b90e03062544c1446eb200040 src/tests/resources_tests.cpp 18e8b670f2f0be83be30b4a73503e786d5ae9b48 src/v1/resources.cpp 4ffe9503b0c792e832dda77e38098509d40e0f18 Diff: https://reviews.apache.org/r/57752/diff/1/ Testing ------- `make check`; added new unit tests both for resource parsing/comparison logic and for end-to-end system behavior (launching a task with a tiny resource value and ensuring it doesn't crash the master). I was concerned that changing `Resources::isEmpty` to introduce a local `Value::Scalar` variable would regress resource parsing performance, but that doesn't appear to be the case. I added a new microbenchmark to validate this; perf w/o the change: ``` [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (741 ms) [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (6138 ms) [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6879 ms total) ``` Perf w/ the change: ``` [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/0 (732 ms) [ RUN ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 [ OK ] Resources_Parse/Resources_Parse_BENCHMARK_Test.Parse/1 (5751 ms) [----------] 2 tests from Resources_Parse/Resources_Parse_BENCHMARK_Test (6483 ms total) ``` (It obviously doesn't improve performance, but these results suggest to me that the change doesn't significantly regress performance, either.) Thanks, Neil Conway --===============2318816429826766581==--