This is an automatically generated e-mail. To reply, visit:

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)

    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)

    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)

    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)

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

src/tests/hierarchical_allocator_tests.cpp (lines 2653 - 2656)

    Technically, allocation may not happen before `AWAIT_READY(allocation);` 
which calls `Clock::settle()`. Let's move this comment *after* the next code 
    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)

    s/background allocation cycle/batch allocation

src/tests/hierarchical_allocator_tests.cpp (line 2683)

    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)

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

    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)

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

src/tests/hierarchical_allocator_tests.cpp (line 2738)

    s/test to//

src/tests/hierarchical_allocator_tests.cpp (lines 2749 - 2760)

    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();
          totalAllocatedResources += Resources::sum(allocation.get().resources);
          currentAllocations[allocation.get().frameworkId] = allocation.get();
          // Recover the allocated resources so they can be offered again next 
          foreachpair (const SlaveID& slaveId,
                       const Resources& resources,
                       allocation.get().resources) {
        ASSERT_EQ(2u, currentAllocations[framework1.id()].resources.size());
        ASSERT_EQ(4u, currentAllocations[framework2.id()].resources.size());
        // Check to ensure that these two allocations sum to the total 
        // this check can ensure there are only two allocations in this case.
    Note I don't explicitly use `expectedFrameworkIds`.

src/tests/hierarchical_allocator_tests.cpp (lines 2772 - 2773)

    Blank line

src/tests/hierarchical_allocator_tests.cpp (lines 2780 - 2782)

    See my comment above about how to avoid numeral suffixes.

src/tests/hierarchical_allocator_tests.cpp (line 2792)

    But `settle()` is necessary : )

src/tests/hierarchical_allocator_tests.cpp (line 2839)

    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