> On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 1982 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line1982> > > > > Use backticks consistently around `sandbox_path`; here and below.
Updated the patch to use single quotes to be consistent with the the already existing tests (`PersistentVolumeDefaultExecutor.ROOT_PersistentResources`). > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 1984 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line1984> > > > > Nice test!! :) Thanks! =) > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 2014 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2014> > > > > Is this needed? This avoids getting gtest warnings if for some reason the nested containers take long to execute and the driver gets another offer. > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 2072-2073 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2072> > > > > This comment is a little confusing. Maybe something like: > > > > "The test will only succeed if the executor and tasks share the same > > volume." I think that tecnically there are 3 volumes, and that even though each one of them is different, the test will pass only if they all map to the same path. I updated the patch using your suggested wording, but we might be able to come up with something better =). > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 2107 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2107> > > > > s/for for/for/ Nooo, not again! :$ > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 2133-2134 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2133> > > > > Is this needed? Nope, removed it from the tests. > On Aug. 30, 2017, 12:49 a.m., Greg Mann wrote: > > src/tests/default_executor_tests.cpp > > Lines 2204-2205 (patched) > > <https://reviews.apache.org/r/61921/diff/1/?file=1804174#file1804174line2204> > > > > 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? I think that we'd still need to add some code, and in the end I don't think that the file would be more readable: This are the things that we'd need to do after the first two tasks run in different task groups: 1) Remove the file created by the "producer". 2) Clear the `taskStages` map between launches. 2) Wait for the executor to commit suicide. 3) Wait for another offer. 4) Set expectations for more updates. 5) Repeat the loop processing the updates. - Gastón ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61921/#review184109 ----------------------------------------------------------- On Aug. 30, 2017, 8:39 p.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, 8:39 p.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/3/ > > > Testing > ------- > > `sudo bin/mesos-tests.sh --gtest_filter="*TasksSharingViaSandboxVolumes*" > --gtest_repeat=500 --gtest_break_on_failure` on GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
