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 Fri, 18 Nov 2016 16:43:11 GMT

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


Fix it, then Ship it!





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

    s/HierarchicalAllocatorCommonRoleQuotaTest/HierarchicalAllocatorTesWithParam/
    
    Yeah it's hard to pick a concise yet descriptive name about the role and the quota, etc.
so just leave it to the comments to explain it?



src/tests/hierarchical_allocator_tests.cpp (lines 3537 - 3538)
<https://reviews.apache.org/r/52295/#comment226522>

    Add a TODO above:
    
    ```
    Move over more allocator tests that make sense to run both when the role is quota'ed and
not. 
    ```



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

    s/MultiFrameworks/QuotaSwitch/
    
    as in, the Bool() is a switch to turn quota on and off?



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

    s/Values(false, true)/Bool()/



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

    s/it is/they are/



src/tests/hierarchical_allocator_tests.cpp (lines 3547 - 3548)
<https://reviews.apache.org/r/52295/#comment226469>

    The correct indentation would be:
    
    ```
    TEST_P(HierarchicalAllocatorTesWithParam,
           AllocateSharedResources)
    {
    }
    ```
    
    "MultiFrameworks" doesn't feel like it's serving a qualifying purpose here (we don't have
another test for a single framework). I'd say we just use the shorter test name "AllocateSharedResources".



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

    s/update/updated/ a better name?



src/tests/hierarchical_allocator_tests.cpp (lines 3609 - 3611)
<https://reviews.apache.org/r/52295/#comment226487>

    Not clear to me what this means...
    
    How about 
    
    ```
    Note that the volume is not recovered as it's used by the task (but it's still offerable
because it's shared).
    ```



src/tests/master_validation_tests.cpp (line 696)
<https://reviews.apache.org/r/52295/#comment226490>

    No need for this variable, just use `sharedDisk` below?



src/tests/master_validation_tests.cpp (line 702)
<https://reviews.apache.org/r/52295/#comment226492>

    s/""/"sleep 1000"/



src/tests/master_validation_tests.cpp (line 710)
<https://reviews.apache.org/r/52295/#comment226494>

    You should be able to do:
    
    ```
    pendingTasks[frameworkId1] = {{task.task_id(), task}};
    ```
    
    and thus don't need the single use variable `taskMap`.



src/tests/master_validation_tests.cpp (line 718)
<https://reviews.apache.org/r/52295/#comment226491>

    Remove a redundant space.



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

    One space before `//`.



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

    This line no longer relevant?



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

    s/likely hood/likelyhood/



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

    No need to wrap `sharedDisk` with `Resources()`?



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

    The meaning of `totalResources` here is not very clear to me: if should have disk of 1000
but here it's 900. Later the `shareDisk` is added to sorter and not to this variable.
    
    Just simply:
    
    ```
      sorter.add(
          slaveId,
          Resources::parse("cpus:100;mem:100;disk(role1):900").get());
    ```
    ?



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

    s/origTotalScalarQuantity/quantity1/ ?
    
    We are applying a sequence of operatoins to the sorter. Names like quantity1, quantity2
and quantity3 are as clear as these long names?



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

    No need to wrap it with `Resources()`?
    
    Here and elsewhere.



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

    Not just "NE", we expect that
    
    ```
    EXPECT_EQ(Resources::parse("disk(role1):100").get(), quantity2 - quantity1);
    ```
    
    right?



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

    "it doesn't get removed from the sorter"
    
    I am being pedantic but the copy does get removed, just not the quantity. How about
    
    ```
    // The quantity of the shared disk is removed only when the last copy is removed.
    ```



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

    Kill this? `sorter.remove(slaveId, Resources(sharedDisk));` is self-explanatory and the
comment doesn't add additional info.


- Jiang Yan Xu


On Nov. 11, 2016, 10:56 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52295/
> -----------------------------------------------------------
> 
> (Updated Nov. 11, 2016, 10:56 a.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 edd5cea8996d7c616cf9428f0f1c05d70c37c307

>   src/tests/master_validation_tests.cpp 7f7bb675b51370036be8edb22ab6cb5f8f913146 
>   src/tests/persistent_volume_tests.cpp 8651b2c5455089041f16d091c90a7e0d948191b8 
>   src/tests/sorter_tests.cpp 9f48abe65c011acfaf79f3d0d79f1d032fd6a4af 
> 
> 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