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 52295: Added additional unit tests for shared resources.
Date Thu, 10 Nov 2016 17:15:08 GMT

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




src/tests/hierarchical_allocator_tests.cpp (lines 1512 - 1513)
<https://reviews.apache.org/r/52295/#comment221467>

    The way this test is written, it's not testing special properties of shared persistent
volumes at all. Persistent volumes have the same behavior...
    
    Shared persistent volumes are offered one-by-one as well but are "offerable even if it's
already in use". We should test that instead.



src/tests/hierarchical_allocator_tests.cpp (line 1519)
<https://reviews.apache.org/r/52295/#comment221424>

    s/with opt/opted in/?



src/tests/hierarchical_allocator_tests.cpp (lines 1530 - 1533)
<https://reviews.apache.org/r/52295/#comment221468>

    Is 3 better than 2 in this case? Can we shorten this test?



src/tests/hierarchical_allocator_tests.cpp (lines 1580 - 1581)
<https://reviews.apache.org/r/52295/#comment225079>

    We don't use this style. Instead:
    
    ```
      EXPECT_EQ(
          allocation.get().resources.get(slave.id()).get().shared(),
          Resources(volume));
    ```



src/tests/hierarchical_allocator_tests.cpp (line 2158)
<https://reviews.apache.org/r/52295/#comment221469>

    This test is basically identitcal to 'AllocateSharedResourcesMultiFrameworks', can we
templatize it?



src/tests/master_validation_tests.cpp (lines 674 - 681)
<https://reviews.apache.org/r/52295/#comment221470>

    Use `createTask()`.



src/tests/master_validation_tests.cpp (lines 683 - 690)
<https://reviews.apache.org/r/52295/#comment221471>

    The existence of `task2` (and `disk2` for that matter) doesn't seem necessary?
    
    They use disctint resources and can't imagine them interfere with each other?
    
    Shorten the test?



src/tests/persistent_volume_tests.cpp (lines 1002 - 1003)
<https://reviews.apache.org/r/52295/#comment225115>

    Can we add how we verify "the master recovers"? e.g.,
    
    ```
    // This test verifies that the master recovers after a failover and re-offers the shared
persistent volume when tasks using the same volume are still running.
    ```



src/tests/persistent_volume_tests.cpp (line 1004)
<https://reviews.apache.org/r/52295/#comment221660>

    s/SharedVolume/SharedPersistentVolume/
    
    Full spelling helps with code grepping.



src/tests/persistent_volume_tests.cpp (lines 1006 - 1008)
<https://reviews.apache.org/r/52295/#comment225098>

    No `masterFlags` necessary.



src/tests/persistent_volume_tests.cpp (line 1011)
<https://reviews.apache.org/r/52295/#comment221472>

    With the new test helper we now use:
    
    ```
    Owned<MasterDetector> detector = master.get()->createDetector();
    Try<Owned<cluster::Slave>> slave = StartSlave(detector.get(), slaveFlags);
    ```



src/tests/persistent_volume_tests.cpp (lines 1050 - 1065)
<https://reviews.apache.org/r/52295/#comment225109>

    Intead of actually writing files, we should probably keep them up via "sleep 1000" so
they are running during the failover.
    
    BTW we need to have expectations for status updates otherwise there are going to be warnings?



src/tests/persistent_volume_tests.cpp (lines 1089 - 1090)
<https://reviews.apache.org/r/52295/#comment225112>

    Nit: The relevant slave behavior can be inferred from the offer so we don't need this
expectation.



src/tests/sorter_tests.cpp (line 637)
<https://reviews.apache.org/r/52295/#comment225610>

    s/disk 50/disk of 50/



src/tests/sorter_tests.cpp (lines 660 - 661)
<https://reviews.apache.org/r/52295/#comment225611>

    This actually fits one line? If not, the indentation should be
    
    ```
      Resources additionalAShared =
        Resources(sharedDisk) + sharedDisk + sharedDisk;
    ```



src/tests/sorter_tests.cpp (line 705)
<https://reviews.apache.org/r/52295/#comment225612>

    s/the fair/its dominant/



src/tests/sorter_tests.cpp (line 722)
<https://reviews.apache.org/r/52295/#comment225614>

    For this test, I think a simple implementation would be to `add()` the same resources
twice and `remove()` it twice. The `totalScalarQuantities()` only drops after the second `remove()`.



src/tests/sorter_tests.cpp (lines 733 - 738)
<https://reviews.apache.org/r/52295/#comment225613>

    Mesos wouldn't add the same shared resources in this way. Call `sorter->add()` multiple
times instead?


- Jiang Yan Xu


On Sept. 26, 2016, 5:39 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> -----------------------------------------------------------
> 
> (Updated Sept. 26, 2016, 5:39 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6185
>     https://issues.apache.org/jira/browse/MESOS-6185
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Tests include the following:
> - Allocator tests when dealing with multiple frameworks of the same
>   role using the same shared volume at the same time.
> - Validity of DESTROY shared volume when there are pending tasks
>   that have been assigned that shared volume.
> - Handlng of master failover.
> - Sorter tests covering shared resources.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 46553f6d501deb37862dd5ba48be1c114f6e6cb8

>   src/tests/master_validation_tests.cpp 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/persistent_volume_tests.cpp 2ab44d11d159162dfcac9d0791b651ed059b8164 
>   src/tests/sorter_tests.cpp 1f17c011898836ea9159661dde7d544cb0d8ea83 
> 
> Diff: https://reviews.apache.org/r/52295/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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