----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61921/#review184109 -----------------------------------------------------------
src/tests/default_executor_tests.cpp Lines 1981 (patched) <https://reviews.apache.org/r/61921/#comment260165> Could you say explicitly here that the tasks in this test are in the same task group? src/tests/default_executor_tests.cpp Lines 1982 (patched) <https://reviews.apache.org/r/61921/#comment260149> Use backticks consistently around `sandbox_path`; here and below. src/tests/default_executor_tests.cpp Lines 1984 (patched) <https://reviews.apache.org/r/61921/#comment260167> Nice test!! :) src/tests/default_executor_tests.cpp Lines 2014 (patched) <https://reviews.apache.org/r/61921/#comment260148> Is this needed? src/tests/default_executor_tests.cpp Lines 2072-2073 (patched) <https://reviews.apache.org/r/61921/#comment260150> This comment is a little confusing. Maybe something like: "The test will only succeed if the executor and tasks share the same volume." src/tests/default_executor_tests.cpp Lines 2106-2108 (patched) <https://reviews.apache.org/r/61921/#comment260154> Fits on two lines. src/tests/default_executor_tests.cpp Lines 2107 (patched) <https://reviews.apache.org/r/61921/#comment260153> s/for for/for/ src/tests/default_executor_tests.cpp Lines 2112-2113 (patched) <https://reviews.apache.org/r/61921/#comment260156> Could you either merge "true" into the first line of the command, or indent the line containing "true" to make it clear that it is part of a single function parameter? src/tests/default_executor_tests.cpp Lines 2119 (patched) <https://reviews.apache.org/r/61921/#comment260158> Could we use a `std::vector` here instead of a C-style array? i.e., ``` vector<Future<v1::scheduler::Event::Update>> updates(4); ``` src/tests/default_executor_tests.cpp Lines 2122-2123 (patched) <https://reviews.apache.org/r/61921/#comment260160> Could you elaborate on why this variable is necessary? src/tests/default_executor_tests.cpp Lines 2133-2134 (patched) <https://reviews.apache.org/r/61921/#comment260163> Is this needed? src/tests/default_executor_tests.cpp Lines 2204-2205 (patched) <https://reviews.apache.org/r/61921/#comment260166> Do you think we should combine these tests into one? The tests are nearly the same, so it would eliminate some duplicated code if we combined them, at the cost of some ambiguity in the case of test failure. What do you think? - Greg Mann On Aug. 30, 2017, 12:39 a.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61921/ > ----------------------------------------------------------- > > (Updated Aug. 30, 2017, 12:39 a.m.) > > > Review request for mesos and Greg Mann. > > > Bugs: MESOS-7916 > https://issues.apache.org/jira/browse/MESOS-7916 > > > Repository: mesos > > > Description > ------- > > These tests verifies that sibling tasks can share a Volume owned by > their parent executor using 'sandbox_path' volumes. > > > Diffs > ----- > > src/tests/default_executor_tests.cpp > afe0afabf784fb65eb833beadd3c584722c321e1 > > > Diff: https://reviews.apache.org/r/61921/diff/2/ > > > Testing > ------- > > `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" > --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
