mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Gastón Kleiman <gas...@mesosphere.io>
Subject Re: Review Request 61921: Added tests to ensure that tasks can access their parent's volumes.
Date Wed, 30 Aug 2017 20:39:32 GMT


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


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