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



Thanks for the follow-up! Let's move this test before all becnhmark tests from 
the `HierarchicalAllocator_BENCHMARK_Test` fixture.


src/tests/hierarchical_allocator_tests.cpp (lines 2607 - 2609)
<https://reviews.apache.org/r/43824/#comment183393>

    I'm an ESL, but having both "per weight" and "by weight" sounds a bit 
strange. Maybe Adam can help wiht finding the right preposition.



src/tests/hierarchical_allocator_tests.cpp (lines 2612 - 2614)
<https://reviews.apache.org/r/43824/#comment183401>

    We try to avoid naming the same entity differently, please use "batch 
allocation" instead of "periodic":
    
    ```
    // Pausing the clock is not necessary, but ensures that the test
    // doesn't rely on the batch allocation in the allocator, which
    // would slow down the test.
    ```



src/tests/hierarchical_allocator_tests.cpp (lines 2631 - 2636)
<https://reviews.apache.org/r/43824/#comment183394>

    Does it fit one line? I know we are a bit inconsistent about it, but let's 
prefer oneliners where it doesn't impact the readability.



src/tests/hierarchical_allocator_tests.cpp (lines 2647 - 2649)
<https://reviews.apache.org/r/43824/#comment183396>

    How about
    
    // Framework2 registers with 'role2' which also uses the default weight. It
    // will not get any offers because all resources are offered to 
`framework1`.



src/tests/hierarchical_allocator_tests.cpp (lines 2653 - 2656)
<https://reviews.apache.org/r/43824/#comment183398>

    Technically, allocation may not happen before `AWAIT_READY(allocation);` 
which calls `Clock::settle()`. Let's move this comment *after* the next code 
block.
    
    It is a minor change, but I think such details are utterly important for 
new people joining the project and reading the code.



src/tests/hierarchical_allocator_tests.cpp (line 2682)
<https://reviews.apache.org/r/43824/#comment183399>

    s/background allocation cycle/batch allocation



src/tests/hierarchical_allocator_tests.cpp (line 2683)
<https://reviews.apache.org/r/43824/#comment183402>

    This doesn't guarantee the allocation happened. Even though the clock will 
be implicitly settled below, let's explicitly add `Clock::settle()` here so 
that the next comment is 100% correct.



src/tests/hierarchical_allocator_tests.cpp (lines 2724 - 2725)
<https://reviews.apache.org/r/43824/#comment183404>

    "Their" is vague. I think you can safely kill the clause and leave just the 
first part of the sentence: "Update the weight of `framework2`'s role to 2.0" 
(mind backticks!)



src/tests/hierarchical_allocator_tests.cpp (lines 2726 - 2728)
<https://reviews.apache.org/r/43824/#comment183408>

    I think these lines are good candidate to go under the `{}` together with 
the checks. This way you can avoid numeral suffixes and have a 1:1 relation 
between `{}`-blocks and test cases.
    
    Does it make sense?



src/tests/hierarchical_allocator_tests.cpp (line 2731)
<https://reviews.apache.org/r/43824/#comment183406>

    Yes, but again we need to `Clock::settle()` for clarity : )



src/tests/hierarchical_allocator_tests.cpp (line 2738)
<https://reviews.apache.org/r/43824/#comment183409>

    s/test to//



src/tests/hierarchical_allocator_tests.cpp (lines 2749 - 2760)
<https://reviews.apache.org/r/43824/#comment183415>

    Let me clarify what I meant in r/41672/. I would like to avoid the 
`if-else` dispatch based on the framework id. It becomes even worse below, 
where we have three frameworks. Here is what I suggest:
    ```
    {
        hashmap<FrameworkID, Allocation> currentAllocations;
        Resources totalAllocatedResources;
    
        for (unsigned i = 0; i < 2; i++) {
          Future<Allocation> allocation = allocations.get();
          AWAIT_READY(allocation);
    
          totalAllocatedResources += Resources::sum(allocation.get().resources);
          currentAllocations[allocation.get().frameworkId] = allocation.get();
    
          // Recover the allocated resources so they can be offered again next 
time.
          foreachpair (const SlaveID& slaveId,
                       const Resources& resources,
                       allocation.get().resources) {
            allocator->recoverResources(
                allocation.get().frameworkId,
                slaveId,
                resources,
                None());
          }
        }
    
        //
        ASSERT_EQ(2u, currentAllocations[framework1.id()].resources.size());
        EXPECT_EQ(Resources::parse(DOUBLE_RESOURCES).get(),
                  
Resources::sum(currentAllocations[framework1.id()].resources));
    
        //
        ASSERT_EQ(4u, currentAllocations[framework2.id()].resources.size());
        EXPECT_EQ(Resources::parse(FOURFOLD_RESOURCES).get(),
                  
Resources::sum(currentAllocations[framework2.id()].resources));
    
        // Check to ensure that these two allocations sum to the total 
resources,
        // this check can ensure there are only two allocations in this case.
        EXPECT_EQ(Resources::parse(TOTAL_RESOURCES).get(), 
totalAllocatedResources);
        EXPECT_EQ(hashset<FrameworkID>(expectedFrameworkIds),
                  currentAllocations.keys());
      }
    ```
    
    Note I don't explicitly use `expectedFrameworkIds`.



src/tests/hierarchical_allocator_tests.cpp (lines 2772 - 2773)
<https://reviews.apache.org/r/43824/#comment183410>

    Blank line



src/tests/hierarchical_allocator_tests.cpp (lines 2780 - 2782)
<https://reviews.apache.org/r/43824/#comment183656>

    See my comment above about how to avoid numeral suffixes.



src/tests/hierarchical_allocator_tests.cpp (line 2792)
<https://reviews.apache.org/r/43824/#comment183658>

    But `settle()` is necessary : )



src/tests/hierarchical_allocator_tests.cpp (line 2839)
<https://reviews.apache.org/r/43824/#comment183391>

    Why do we need to `resume` here?


- Alexander Rukletsov


On March 1, 2016, 6:42 a.m., Yongqiao Wang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43824/
> -----------------------------------------------------------
> 
> (Updated March 1, 2016, 6:42 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4200
>     https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Addressed comments of 41672.
> 
> 
> Diffs
> -----
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 5f771f02db9bd098f3cd36730cd84bf2f5e87a33 
> 
> Diff: https://reviews.apache.org/r/43824/diff/
> 
> 
> Testing
> -------
> 
> make && make check successfully.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Reply via email to