mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Michael Park <mp...@apache.org>
Subject Re: Review Request 55828: Updated Resources::apply to handle Resource.AllocationInfo.
Date Sun, 29 Jan 2017 08:02:02 GMT

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


Fix it, then Ship it!




It took me a while trying to figure out what the invariants of `Resources` and `Offer::Operation`
are, but I think I have a grasp of it.

- We have `recovered.apply(operation);` where we have `recovered` and `operation` both with
unallocated resources. (after `recovered` adjustment)
- We have a few `offeredResources.apply(operation);` where we have `offeredResources` and
`operation` both with resources allocated to a single role. (after `operation` adjustment)
- We also have `totalResources.apply(operation);` where we have `totalResources` with unallocated
resources, and `operation` with resources allocated to a single role. (case 2 helps us with
this)

It seems to me like we really end up with resources allocated to __multiple__ roles if we
were to allow accepting multiple offers from multiple roles.
That is, currently the resources in `Resources` and `Offer::Operation` are either all unallocated,
or all allocated to one role.

Is this accurate?


src/common/resources.cpp (lines 1257 - 1261)
<https://reviews.apache.org/r/55828/#comment234884>

    We talked about how this condition probably isn't necessary since we set the `AllocationInfo`
in
    `Offer::Operation`s in the master (and redundantly in the allocator).
    
    Please let me know how that turned out!



src/common/resources.cpp (line 1339)
<https://reviews.apache.org/r/55828/#comment234829>

    Shouldn't we print `unreserved` as opposed to `adjustedReservation` here?


- Michael Park


On Jan. 23, 2017, 2:47 p.m., Benjamin Mahler wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55828/
> -----------------------------------------------------------
> 
> (Updated Jan. 23, 2017, 2:47 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael Park.
> 
> 
> Bugs: MESOS-6967
>     https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an allocation role.
> The two interesting cases for adjusting the operation's resource are:
> 
> (1) The operation `Resource` does not contain an `AllocationInfo`
>     but is being applied to an allocated `Resources`. We allow this
>     only if the operation is unambiguous, that is, the allocated
>     `Resources` are only allocated to a single role.
> 
> (2) The operation `Resource` contains an `AllocationInfo` but is
>     being applied to an unallocated `Resources`. In this case we
>     simply ignore the `AllocationInfo` of the `Resource`.
> 
> Note that we assume no `Resources` store a mix of allocated and
> unallocated resources. This is a brittle assumption that we should
> have enforcement for.
> 
> 
> Diffs
> -----
> 
>   src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 
>   src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 
>   src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 
>   src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 
> 
> Diff: https://reviews.apache.org/r/55828/diff/
> 
> 
> Testing
> -------
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>


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