mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 57964: Added a test to verify metrics when shared resources are present.
Date Fri, 21 Apr 2017 22:06:33 GMT

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




src/tests/persistent_volume_tests.cpp
Lines 1174-1175 (patched)
<https://reviews.apache.org/r/57964/#comment245767>

    The comment doesn't seem right?



src/tests/persistent_volume_tests.cpp
Lines 1176 (patched)
<https://reviews.apache.org/r/57964/#comment245738>

    s/SharedVolume/SharedPersistentVolume/
    
    for consistent with the other tests (for greppability).
    
    In fact, given
    
    ```
    912:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMultipleTasks)
    1022:TEST_P(PersistentVolumeTest, SharedPersistentVolumeRescindOnDestroy)
    1177:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMasterFailover)
    1525:TEST_P(PersistentVolumeTest, SharedPersistentVolumeMultipleIterations)
    ```
    
    Perhaps "SharedPersistentVolumeMultipleFrameworks" or "SharedPersistentVolumeMetricsMultipleFrameworks"?
    
    It looks like we didn't have a test that covers multiple frameworks launching tasks onto
the same volume so I feel that "MultipleFrameworks" is worth mentioning. And frankly we are
testing more than just the metrics: we are testing the two tasks can both write the the same
volume, etc. So this is just a general "multiple tasks from multiple frameworks" test.
    
    We can make the intention of this test clear in the comment above.



src/tests/persistent_volume_tests.cpp
Lines 1211-1212 (patched)
<https://reviews.apache.org/r/57964/#comment245741>

    I see `Clock::advance(slaveFlags.registration_backoff_factor);` is for the agent's initial
backoff but the allocation should be immediate without clock advancement?



src/tests/persistent_volume_tests.cpp
Lines 1230-1232 (patched)
<https://reviews.apache.org/r/57964/#comment245742>

    The test below does use the volume and depend on the volume being created and in this
test I guess we have two legit tasks using the volume so perhaps just kill the last sentence?



src/tests/persistent_volume_tests.cpp
Lines 1255-1256 (patched)
<https://reviews.apache.org/r/57964/#comment245768>

    Actually this doesn't look necessary, we already wait for the task status update below.



src/tests/persistent_volume_tests.cpp
Lines 1261 (patched)
<https://reviews.apache.org/r/57964/#comment245744>

    s/framework 1/framework1/?



src/tests/persistent_volume_tests.cpp
Lines 1263-1266 (patched)
<https://reviews.apache.org/r/57964/#comment245770>

    Nit: Strictly speaking I think all the `stats1.values.count` ones should be `ASSERT_EQ`
(these should always be there and you should abort the test if they didn't) but I see that
it's not done consistently.



src/tests/persistent_volume_tests.cpp
Lines 1298 (patched)
<https://reviews.apache.org/r/57964/#comment245747>

    You did `EXPECT_FALSE(offers1->empty());` above, do the same here?



src/tests/persistent_volume_tests.cpp
Lines 1307 (patched)
<https://reviews.apache.org/r/57964/#comment245748>

    "a portion of the offered resources" doesn't have any significance here so we can simplify
the comment? Here what's worth pointing out is just "task2 uses the same shared volume as
task1".



src/tests/persistent_volume_tests.cpp
Lines 1313 (patched)
<https://reviews.apache.org/r/57964/#comment245764>

    "sleep 1000" above but "sleep 500" here? Keep them consistent ("sleep 1000")?



src/tests/persistent_volume_tests.cpp
Lines 1326-1327 (patched)
<https://reviews.apache.org/r/57964/#comment245769>

    Actually this doesn't look necessary, we already wait for the task status update below.


- Jiang Yan Xu


On March 27, 2017, 3:27 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57964/
> -----------------------------------------------------------
> 
> (Updated March 27, 2017, 3:27 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7186
>     https://issues.apache.org/jira/browse/MESOS-7186
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added a test to verify metrics when shared resources are present.
> 
> 
> Diffs
> -----
> 
>   src/tests/persistent_volume_tests.cpp 3eb7afe3de8e72ffb6502dfe12ef37ccd4ca2125 
> 
> 
> Diff: https://reviews.apache.org/r/57964/diff/2/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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