You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Jiang Yan Xu <ya...@jxu.me> on 2016/12/12 09:30:18 UTC
Re: Review Request 51027: Track allocation candidates to bound
allocator.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review158818
-----------------------------------------------------------
This patch itself LGTM. Will give ship it once we confirm the flaky tests are fixed and we have at least minimal test for its efficacy.
src/master/allocator/mesos/hierarchical.hpp (line 224)
<https://reviews.apache.org/r/51027/#comment229654>
To make it more clear to the reader, we can explain what the future means:
```
The returned future becomes ready when the latest allocation run which commences after this call has completed.
NOTE: At any given moment there's at most one allocation run pending or being executed.
```
src/master/allocator/mesos/hierarchical.hpp (line 227)
<https://reviews.apache.org/r/51027/#comment229655>
The following should be sufficient.
```
// Method that performs the allocation work.
```
src/master/allocator/mesos/hierarchical.cpp (lines 1281 - 1282)
<https://reviews.apache.org/r/51027/#comment229657>
Does this work?
```
return allocate({slaveId});
```
src/master/allocator/mesos/hierarchical.cpp (lines 1297 - 1298)
<https://reviews.apache.org/r/51027/#comment229636>
"allocation-triggering events" better than "events triggering allocations"?
src/master/allocator/mesos/hierarchical.cpp (line 1299)
<https://reviews.apache.org/r/51027/#comment229637>
s/a single slaveId/a single `slaveId`/ to be consistent.
src/master/allocator/mesos/hierarchical.cpp (lines 1334 - 1338)
<https://reviews.apache.org/r/51027/#comment229638>
Clear the candidates after the log line because you need `allocationCandidates.size()`?
src/master/allocator/mesos/hierarchical.cpp (line 1347)
<https://reviews.apache.org/r/51027/#comment229652>
Move this to `run()`, above `Stopwatch stopwatch;`.
- Jiang Yan Xu
On Oct. 18, 2016, 9:14 a.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 9:14 a.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2c31471ee0f5d6836393bf87ff9ecfd8df835013
> src/master/allocator/mesos/hierarchical.cpp c8f9492ee1b69e125a1e841116d22a578a9b524e
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check
>
> note: check without filters depends on https://reviews.apache.org/r/51028 and https://reviews.apache.org/r/52534
>
>
> Thanks,
>
> Jacob Janco
>
>
Re: Review Request 51027: Track allocation candidates to bound
allocator.
Posted by Jacob Janco <jj...@gmail.com>.
> On Dec. 12, 2016, 9:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1307
> > <https://reviews.apache.org/r/51027/diff/10/?file=1540692#file1540692line1307>
> >
> > s/a single slaveId/a single `slaveId`/ to be consistent.
Changed the wording to describe the events in plain english, previously I was describing methods.
// Events that trigger allocations, e.g. adding an agent or a framework,
// update the set of `allocationCandidates` with a single agent in the
// former case and the set of all known agents in the latter.
> On Dec. 12, 2016, 9:30 a.m., Jiang Yan Xu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 1287-1288
> > <https://reviews.apache.org/r/51027/diff/10/?file=1540692#file1540692line1287>
> >
> > Does this work?
> >
> > ```
> > return allocate({slaveId});
> > ```
No, the hashset constructor allows an initializer list, I think `return allocate({slaveId});` would just scope `slaveId`.
- Jacob
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51027/#review158818
-----------------------------------------------------------
On Oct. 18, 2016, 4:14 p.m., Jacob Janco wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51027/
> -----------------------------------------------------------
>
> (Updated Oct. 18, 2016, 4:14 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Guangya Liu, James Peach, Klaus Ma, and Jiang Yan Xu.
>
>
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
>
>
> Repository: mesos
>
>
> Description
> -------
>
> - Triggered allocations dispatch allocate() only
> if there is no pending allocation in the queue.
> - Allocation candidates are accumulated and only
> cleared when enqueued allocations are processed.
>
>
> Diffs
> -----
>
> src/master/allocator/mesos/hierarchical.hpp 2c31471ee0f5d6836393bf87ff9ecfd8df835013
> src/master/allocator/mesos/hierarchical.cpp c8f9492ee1b69e125a1e841116d22a578a9b524e
>
> Diff: https://reviews.apache.org/r/51027/diff/
>
>
> Testing
> -------
>
> make check
>
> note: check without filters depends on https://reviews.apache.org/r/51028 and https://reviews.apache.org/r/52534
>
>
> Thanks,
>
> Jacob Janco
>
>