mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 61921: Added tests to ensure that tasks can access their parent's volumes.
Date Wed, 30 Aug 2017 00:49:51 GMT

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


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