----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55828/#review163503 -----------------------------------------------------------
src/common/resources.cpp (lines 1263 - 1266) <https://reviews.apache.org/r/55828/#comment234974> Could you clarify under what conditions this can happen? It seems surprising to silently drop information explicitly given by users, and I believe rejecting the operation would be clearer. If situations can occur where a user has more recent information than the current master (e.g., an allocation info not yet known to the master) we should fix failover behavior. src/common/resources.cpp (lines 1268 - 1270) <https://reviews.apache.org/r/55828/#comment234973> `..., see MESOS-6636.` to allow better tracking of the status of this. src/common/resources.cpp (line 1272) <https://reviews.apache.org/r/55828/#comment234971> Did you intend to make this `const`? If not, the following appears more straight-forward to me, bool isAllocated = false; foreach(const Resource& resource, resources) { if (resource.has_allocation_info()) { isAllocated = true; break; } } If you'd went for the optimization suggested by Guangya this does become a "one-liner" though, const bool isAllocated = resources.resources().empty() ? false : resources.resources(0).has_allocation_info(); src/common/resources.cpp (line 1288) <https://reviews.apache.org/r/55828/#comment234972> This looks like an expensive computation. Should we precompute it at function scope and capture it here by ref? src/tests/resources_utils.hpp (line 40) <https://reviews.apache.org/r/55828/#comment234969> This class seems confusing. It e.g., is not a wrapper since when constructing an `AllocatedResources` from some unallocated `Resources` an assertion like `EXPECT_EQ(resources, allocated)` is not true (i.e., it doesn't just wrap some `Resources` and some role). What about making this a function for now, e.g., Resources allocatedResources( const Resources& resources, const string& role); - Benjamin Bannier On Jan. 23, 2017, 11: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, 11: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 > >