----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57752/#review169916 -----------------------------------------------------------
Ship it! Ship It! - Joris Van Remoortere On March 19, 2017, 4:21 a.m., Neil Conway wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57752/ > ----------------------------------------------------------- > > (Updated March 19, 2017, 4:21 a.m.) > > > 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 > >
