mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 51027: Track allocation candidates to bound allocator.
Date Thu, 19 Jan 2017 02:45:02 GMT

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


Fix it, then Ship it!




The logic looks good to me, just a few minor comments.


src/master/allocator/mesos/hierarchical.hpp (lines 221 - 229)
<https://reviews.apache.org/r/51027/#comment233533>

    I think it's sufficient here to just say:
    
    ```
    // Allocate resources from the specified agents.
    ```
    
    The rest of the comment seems to be a re-iteration of the logic inside the implementations.
Do we need to say that the other two call into this? Or how exactly we do the batching? IMHO
it's likely to go stale over time and the reader can read the implementation to see exactly
how it works.
    
    We could add something like:
    
    ```
      // Allocate any allocatable resources from all known agents,
      // the allocation is deferred and batched with other
      // allocation requests.
      process::Future<Nothing> allocate();
    ```
    
    But it seems unnecessary given the caller just needs to understand that it is asynchronous
(i.e. returns a Future).



src/master/allocator/mesos/hierarchical.hpp (lines 232 - 236)
<https://reviews.apache.org/r/51027/#comment233536>

    Run seems a bit generic. How about `_allocate()` and `__allocate()` to make it clear it's
the first `_allocate()` continuation?



src/master/allocator/mesos/hierarchical.cpp (lines 1267 - 1274)
<https://reviews.apache.org/r/51027/#comment233544>

    This change introduces an additional trip through the allocator's queue after the allocation
completes and before the delay is called.
    
    This would avoid it:
    
    ```
    auto pid = self();
    
    // TODO: Use process::loop for the allocation loop.
    allocate()
      .onAny([pid]() {
        delay(allocationInterval, self(), &Self::batch);
      }
    ```
    
    Not sure if this was intentional or not.



src/master/allocator/mesos/hierarchical.cpp (lines 1301 - 1305)
<https://reviews.apache.org/r/51027/#comment233545>

    This seems pretty self explanatory, no need for this comment IMHO. The example is likely
to go stale.



src/master/allocator/mesos/hierarchical.cpp (lines 1308 - 1309)
<https://reviews.apache.org/r/51027/#comment233546>

    Why the newline?


- Benjamin Mahler


On Jan. 12, 2017, 6:55 p.m., Jacob Janco wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
> 
> (Updated Jan. 12, 2017, 6:55 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang
Yan Xu.
> 
> 
> Bugs: MESOS-6904
>     https://issues.apache.org/jira/browse/MESOS-6904
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> - Triggered allocations dispatch allocate() only
>   if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
>   cleared when enqueued allocations are processed.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.hpp a6424d624864155e1c87a28a63b784512c5c8722

>   src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca

> 
> Diff: https://reviews.apache.org/r/51027/diff/
> 
> 
> Testing
> -------
> 
> make check with the filters below
> 
> Broken tests: 
> - TEST_F(HierarchicalAllocatorTest, SuppressAndReviveOffers), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunsMetric), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, AllocationRunTimerMetrics), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, UpdateWeight), fix in 51028
> - TEST_P(HierarchicalAllocator_BENCHMARK_Test, AddAndUpdateSlave), fix in 51028
> - TEST_F(HierarchicalAllocatorTest, SmallOfferFilterTimeout), fix in 52534
> - TEST_F(OversubscriptionTest, RescindRevocableOfferWithIncreasedRevocable), fix in 51621
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>


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