mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 43824: Addressed comments of 41672.
Date Thu, 03 Mar 2016 09:03:27 GMT

-----------------------------------------------------------
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
> 
>


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