You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/02/01 22:45:35 UTC

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

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

(Updated Feb. 1, 2017, 10:45 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
-------

Rebased after batch allocation patch was merged.


Bugs: MESOS-6444
    https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
-------

We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.


Diffs (updated)
-----

  src/master/allocator/mesos/hierarchical.cpp ffa07087f9ed8d28b99cc4cde7c739cfd7edb1e1 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
-------

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review164578
-----------------------------------------------------------


Ship it!




Ship It!

- Jiang Yan Xu


On Feb. 6, 2017, 9:02 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
> 
> (Updated Feb. 6, 2017, 9:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
>     https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363 
> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------

(Updated Feb. 7, 2017, 5:02 a.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Changes
-------

Addressed comment and rebased.


Bugs: MESOS-6444
    https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
-------

We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.


Diffs (updated)
-----

  src/master/allocator/mesos/hierarchical.cpp 56d6791baa64189523df668749f4a7ab67d6b363 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
-------

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/#review164101
-----------------------------------------------------------



LGTM. Just need to take another look after rebasing to ship it.


src/master/allocator/mesos/hierarchical.cpp (lines 682 - 683)
<https://reviews.apache.org/r/55359/#comment236137>

    "contained" is ambiguous, I think it's better if we convey the idea of "at least one copy".
    
    How about:
    
    ```
    Ensure that offered resources contain at least one copy of the consumed shared resource (guaranteed by master validation).
    ```



src/master/allocator/mesos/hierarchical.cpp (lines 696 - 697)
<https://reviews.apache.org/r/55359/#comment236123>

    So `additional` now encompasses multiple launch operations but it feels like it's still valuable to log the taskIds in this call for debugging?



src/master/allocator/mesos/hierarchical.cpp (lines 732 - 733)
<https://reviews.apache.org/r/55359/#comment236135>

    So now we have two reasons:
    
    ```
    The agent's total resources shouldn't contain:
    1. The additionally allocated shared resources.
    2. `AllocationInfo` as set in `updatedOfferedResources`.
    ```
    
    Maybe spell them out like this?


- Jiang Yan Xu


On Feb. 3, 2017, 2:01 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55359/
> -----------------------------------------------------------
> 
> (Updated Feb. 3, 2017, 2:01 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6444
>     https://issues.apache.org/jira/browse/MESOS-6444
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> We handle shared resources for `LAUNCH` operations separately in the
> `updateAllocation()` API to add additional copies of shared resources.
> But the updates to the allocations in the allocator and sorters
> can be consolidated together.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 5f540569043df9d2bb75416c8c36bb4dd7bd68a1 
> 
> Diff: https://reviews.apache.org/r/55359/diff/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 55359: Consolidate update of allocations in `updateAllocation()`.

Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55359/
-----------------------------------------------------------

(Updated Feb. 3, 2017, 10:01 p.m.)


Review request for mesos, Benjamin Mahler and Jiang Yan Xu.


Bugs: MESOS-6444
    https://issues.apache.org/jira/browse/MESOS-6444


Repository: mesos


Description
-------

We handle shared resources for `LAUNCH` operations separately in the
`updateAllocation()` API to add additional copies of shared resources.
But the updates to the allocations in the allocator and sorters
can be consolidated together.


Diffs (updated)
-----

  src/master/allocator/mesos/hierarchical.cpp 5f540569043df9d2bb75416c8c36bb4dd7bd68a1 

Diff: https://reviews.apache.org/r/55359/diff/


Testing
-------

Tests passed.


Thanks,

Anindya Sinha