mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Neil Conway <neil.con...@gmail.com>
Subject Review Request 57752: Fixed crash with tasks that use very small resource values.
Date Sun, 19 Mar 2017 04:21:58 GMT

-----------------------------------------------------------
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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message