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

Reply via email to