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