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 55359: Consolidate update of allocations in `updateAllocation()`.
Date Thu, 26 Jan 2017 01:11:26 GMT

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



My feeling is that we were using too many variables for the same thing and after this review
there are still more than we need.


Feels like there should just be these variables to capture the allocation change:

`offeredResources`: passed down from the master, this is the "original".
`updatedOfferedResources`: the version with all the operations applied, either in `Resources::apply()`,
or within this method for `LAUNCH`. This is the "updated", but more clear, since we are dealing
with both total resources and allocations.

Then we can update all the allocation related varaibles using these two.

Then, next, we update the agent total resources. Why updating the totals in the method `updateAllocation()`?
We can clarify this in the method (see my comments there).


src/master/allocator/mesos/hierarchical.cpp (lines 640 - 652)
<https://reviews.apache.org/r/55359/#comment234467>

    To unify these variables, perhaps don't use `_offeredResources`. To be more consistent
with the naming pattern within this method (`something` and `updatedSomething`), perhaps:
    
    ```
    Resources updatedOfferedResources = offeredResources;
    
    // (The objective of this loop is to obtain the correct `updatedOfferedResources`.)
    foreach (const Offer::Operation& operation, operations) {  
      if (operation.type() == Offer::Operation::LAUNCH) {
        // LAUNCH custom logic.
        // (Here everything we do is essentially also udpate `updatedOfferedResources`.)
        // ...
      } else {
        // (Here we invoke `Resources::apply()` to update `updatedOfferedResources`.)
        Try<Resources> _updatedOfferedResources = updatedOfferedResources.apply(operation);
        CHECK_SOME(_updatedOfferedResources);
        updatedOfferedResources = _updatedOfferedResources.get();
      }
    }
    
    // At this point, we are outside the loop and we have "fully" processed the offered
    // resources to obtain `updatedOfferedResources`.
    // Now use it to update allocator and sorter variables.
    
    // ...
    ```
    
    We don't need `updated` to capture the updated offered resources again.



src/master/allocator/mesos/hierarchical.cpp (line 645)
<https://reviews.apache.org/r/55359/#comment234431>

    `original` is updated in every iteration?
    
    Isn't original offered resources the variable `offeredResources`?



src/master/allocator/mesos/hierarchical.cpp (lines 697 - 699)
<https://reviews.apache.org/r/55359/#comment234469>

    I see you've pulled this up so we don't have to get the same `frameworkAllocation` in
every iteration.
    
    However perhaps we can do it with just `offeredResources`:
    
    ```
    foreach (const Resource& resource, additional) {
      CHECK(offeredResources.contains(resource));
    }
    ```
    
    Indeed, the rule is more precisely "we allow tasks to request additional allocations if
the (original) offered resources already contain the shared resources", right?



src/master/allocator/mesos/hierarchical.cpp (lines 704 - 712)
<https://reviews.apache.org/r/55359/#comment234470>

    This doesn't seem to need to go inside the `foreach (const Offer::Operation& operation,
operations)` loop? The launch operation is a no-op with LAUNCH. If outside the loop, we can
just do
    
    ```
    // ... update allocation-related variables first.
    
    // Update the agent total resources so they are consistent with the updated allocation.

    // We don't directly use `updatedOfferedResources` here because we don't count 
    // additionally allocated resources in the agent total resources. (See comments in `updateSlaveTotal`.)
    Try<Resources> updatedTotal = slaves[slaveId].total.apply(operations);
    CHECK_SOME(updatedTotal);
    updateSlaveTotal(slaveId, updatedTotal.get());
    ```



src/master/allocator/mesos/hierarchical.cpp (lines 748 - 749)
<https://reviews.apache.org/r/55359/#comment234476>

    After all operations perhaps it's valuable to CHECK that we indeed didn't change the quantities.
So for this we can save the framework's allocation at the very beginning and compare with
it at the very end.
    
    ```
    void HierarchicalAllocatorProcess::updateAllocation(
        const FrameworkID& frameworkId,
        const SlaveID& slaveId,
        const Resources& offeredResources,
        const vector<Offer::Operation>& operations)
    {
      // ... some initial checks.
      
      // (Save `frameworkAllocation`.)
      const Resources frameworkAllocation = 
        frameworkSorter->allocation(frameworkId.value(), slaveId);
    
      // Do the work.
      
      // Check that the quantities are not changed.
      const Resources updatedFrameworkAllocation = frameworkSorter->allocation(frameworkId.value(),
slaveId);
      CHECK_EQ(frameworkAllocation.createStrippedScalarQuantity(),
               updatedFrameworkAllocation.createStrippedScalarQuantity());
               
      LOG(INFO) << "Updated allocation of framework " << frameworkId
                << " on agent " << slaveId
                << " from " << frameworkAllocation
                << " to " << updatedFrameworkAllocation;
    }
    ```


- Jiang Yan Xu


On Jan. 9, 2017, 2:37 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
> 
> (Updated Jan. 9, 2017, 2:37 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
>     https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 91b1ec43940a788459f045ca4a4b82d4e8373bca

> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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