mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joris Van Remoortere <joris.van.remoort...@gmail.com>
Subject Re: Review Request 57752: Fixed crash with tasks that use very small resource values.
Date Thu, 23 Mar 2017 19:15:46 GMT

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


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