mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Chun-Hung Hsiao <chhs...@apache.org>
Subject Re: Review Request 66227: Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.
Date Wed, 18 Apr 2018 01:25:26 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66227/#review201366
-----------------------------------------------------------




src/tests/api_tests.cpp
Lines 3688 (patched)
<https://reviews.apache.org/r/66227/#comment282564>

    Let's try to split the test into two so each test verifies one operation.
    
    Also, what's the reason of disabling the test on Windows?
    
    And do you think it is reasonable to have tests that leave `agent_id` unset, to expose
the code path of validating the existence of `agent_id`?



src/tests/api_tests.cpp
Lines 3692-3695 (patched)
<https://reviews.apache.org/r/66227/#comment282566>

    This is not needed. The overloaded `MasterAPITest::CreateMasterFlags()` already extends
the allocation interval.



src/tests/api_tests.cpp
Lines 3700 (patched)
<https://reviews.apache.org/r/66227/#comment282567>

    Since the API is not called XXXVolumes, let's just say
    ```
    // ... use it in the API calls.
    ```
    so need to break the line here.



src/tests/api_tests.cpp
Lines 3711-3719 (patched)
<https://reviews.apache.org/r/66227/#comment282569>

    No need to add the `RESOURCE_PROVIDER` capability since this is set up by default in 1.6.
In fact, we should remove it here so if the default capabilities are changed in the future,
we will be able to capture the problem through this test.



src/tests/api_tests.cpp
Lines 3750 (patched)
<https://reviews.apache.org/r/66227/#comment282570>

    Let's move this to the beginning of this test.



src/tests/api_tests.cpp
Lines 3766 (patched)
<https://reviews.apache.org/r/66227/#comment282572>

    Not sure if this is necessary. Could you justify the need of this synchronization?



src/tests/api_tests.cpp
Lines 3768 (patched)
<https://reviews.apache.org/r/66227/#comment282573>

    Is this necessary?



src/tests/api_tests.cpp
Lines 3787 (patched)
<https://reviews.apache.org/r/66227/#comment282574>

    Do we need this? IIRC an event-based allocation will be triggered (and thus an offer will
be sent) when a framework is added.



src/tests/api_tests.cpp
Lines 3791 (patched)
<https://reviews.apache.org/r/66227/#comment282575>

    ```
    Offer offer = offersAfterCreate->at(0);
    ```



src/tests/api_tests.cpp
Lines 3801-3809 (patched)
<https://reviews.apache.org/r/66227/#comment282576>

    Let's do the following for readability:
    ```
    v1::master::Call::GrowVolume* growVolume =
      v1GrowVolumeCall.mutable_grow_volume();
    growVolume->mutable_agent_id()->CopyFrom(evolve(slaveId));
    growVolume->mutable_volume()->CopyFrom(evolve(volume));
    growVolume->mutable_addition)->CopyFrom(evolve(addition));
    ```



src/tests/api_tests.cpp
Lines 3821 (patched)
<https://reviews.apache.org/r/66227/#comment282578>

    No need for this since the clock is paused.



src/tests/api_tests.cpp
Lines 3834 (patched)
<https://reviews.apache.org/r/66227/#comment282577>

    Again, I'm not convinced that this is required.



src/tests/api_tests.cpp
Lines 3836 (patched)
<https://reviews.apache.org/r/66227/#comment282579>

    Do we need this?



src/tests/api_tests.cpp
Lines 3841 (patched)
<https://reviews.apache.org/r/66227/#comment282580>

    ```
    offer = offersAfterGrow->at(0);
    ```



src/tests/api_tests.cpp
Lines 3859-3866 (patched)
<https://reviews.apache.org/r/66227/#comment282581>

    ```
    v1::master::Call::ShrinkVolume* shrinkVolume =
      v1ShrinkVolumeCall.mutable_shrink_volume();
    shrinkVolume->...
    ```



src/tests/api_tests.cpp
Lines 3878 (patched)
<https://reviews.apache.org/r/66227/#comment282583>

    No need for this as the clock is paused.



src/tests/api_tests.cpp
Lines 3891 (patched)
<https://reviews.apache.org/r/66227/#comment282582>

    Again, not sure if this is needed.



src/tests/api_tests.cpp
Lines 3893 (patched)
<https://reviews.apache.org/r/66227/#comment282584>

    Also it seems to me that this is not needed.



src/tests/api_tests.cpp
Lines 3898 (patched)
<https://reviews.apache.org/r/66227/#comment282585>

    ```
    offer = offerAfterShrink->at(0);
    ```



src/tests/api_tests.cpp
Lines 3903-3945 (patched)
<https://reviews.apache.org/r/66227/#comment282586>

    If I'm not mistaken, the test teardown would clean up the work dir, and thus the persistent
volume, so this is not required. Can you verify that?


- Chun-Hung Hsiao


On April 11, 2018, 9:21 p.m., Zhitao Li wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66227/
> -----------------------------------------------------------
> 
> (Updated April 11, 2018, 9:21 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao, Gaston Kleiman, and Greg Mann.
> 
> 
> Bugs: MESOS-8747
>     https://issues.apache.org/jira/browse/MESOS-8747
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test for `GROW_VOLUME` and `SHRINK_VOLUME` operator API.
> 
> 
> Diffs
> -----
> 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
> 
> 
> Diff: https://reviews.apache.org/r/66227/diff/4/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>


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