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 55828: Updated Resources::apply to handle Resource.AllocationInfo.
Date Wed, 01 Feb 2017 22:40:24 GMT


> On Jan. 28, 2017, 3:35 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, lines 1274-1276
> > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1274>
> >
> >     This indicates that if the `Resource` do not have `AllocationInfo`, then we
will loop all of the `Resource` till finished then return false, but we have the assumption
of no `Resources` store a mix of allocated and unallocated resources, so how about `return
resource.has_allocation_info();` here?

I was a bit hestitant to say it's not allocated if one but not all of the resources have allocation
info. I agree though that we need to enforce the invariant or have separate types for allocated
and unallocated resources to prevent mixing, at which point we can write the more efficient
version.


- Benjamin


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


On Jan. 23, 2017, 10: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, 10: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