Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Michael Park]

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


Fix it, then Ship it!




I think this is ready to go.


src/master/allocator/sorter/drf/sorter.hpp
Lines 109 (patched)


"sorter's tree"?



src/master/allocator/sorter/drf/sorter.hpp
Lines 141 (patched)


`s/role paths/client paths/`?



src/master/allocator/sorter/drf/sorter.hpp
Lines 187 (patched)


"sorter's tree" here as well.



src/master/allocator/sorter/drf/sorter.hpp
Lines 253 (patched)


mm... I feel like maybe my suggestion of using `clientPath` isn't great.. 
since calling this function on a non-client will return its path, but the 
result is not a "client path".

Maybe you were right with `logicalPath`?


- Michael Park


On April 20, 2017, 9:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated April 20, 2017, 9:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree; the
> tree represents the hierarchical relationship between sorter clients.
> Each node in the tree contains a vector of pointers to child nodes. The
> tree might contain nodes that do not correspond directly to sorter
> clients. For example, adding clients "a/b" and "c/d" results in the
> following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each active
> client in the sorter. This is implemented by ensuring that each sorter
> client is associated with a leaf node in the tree (i.e., internal nodes
> are not returned by `sort`). Note that it is possible for a leaf node to
> be transformed into an internal node by a subsequent insertion; to
> handle this case, we "implicitly" create an extra child node, which
> maintains the invariant that each client is associated with a leaf
> node. For example, if the client "a/b/x" is added to the tree above, the
> result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x". The "." node is called a "virtual leaf node".
> 
> This commit also introduces a new approach to sorting: rather than
> keeping a `std::set` of sorter clients, we now keep a tree of
> `std::vector`, which is sorted explicitly via `std::sort` when
> necessary. The previous implementation tried to optimize the sorting
> process by updating the sort order incrementally when a single sorter
> client was updated; this commit removes that optimization, and instead
> re-sorts the entire tree whenever a change is made that might alter the
> sort order. Re-introducing a version of this optimization would make
> sense in the future (MESOS-7390), but benchmarking suggests that this
> commit results in a net improvement in sorter performance for
> non-hierarchical clients, anyway. The performance improvement is likely
> due to the introduction of a secondary hashmap that allows the leaf node
> associated with a client name to be found efficiently; the previous
> implementation required a linear scan of a `std::set`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
>   src/tests/master_allocator_tests.cpp 
> 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
>   src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/24/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 57730: Fixed example tests which broke due to the new `register_agents` ACL.

[Jiang Yan Xu]

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

(Updated April 20, 2017, 10:10 p.m.)


Review request for mesos and Anindya Sinha.


Changes
---

Comments.


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


Repository: mesos


Description
---

- With the new `register_agents` ACL, `permissive == false` would lead
  to the agent unable to register unless explicitly allowed.


Diffs (updated)
-

  src/tests/script.cpp 3b68b845b06fe19acb8b08e1ff3dd0bec9117f05 


Diff: https://reviews.apache.org/r/57730/diff/2/

Changes: https://reviews.apache.org/r/57730/diff/1-2/


Testing
---

make check


Thanks,

Jiang Yan Xu

Re: Review Request 57730: Fixed example tests which broke due to the new `register_agents` ACL.

[Jiang Yan Xu]

> On April 11, 2017, 2:11 a.m., Adam B wrote:
> > src/tests/script.cpp
> > Lines 161 (patched)
> > 
> >
> > We don't even enable agent authentication in most(/any?) tests, so I'd 
> > expect the agents to try to register without authentication. Should we 
> > enable agent authentication (and authorization) by default in the tests, 
> > and only disable it for the rare few tests that don't need it?
> 
> Jiang Yan Xu wrote:
> Good point but this patch allows the current tests to run without making 
> things worse. For this improvement we should probably separate the agent 
> credentials and the framework credentials in the examples which I am happy to 
> do. A TODO here and a followup patch sound good?

Updated comments and filed https://issues.apache.org/jira/browse/MESOS-7408 to 
track.


- Jiang Yan


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


On April 20, 2017, 10:10 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57730/
> ---
> 
> (Updated April 20, 2017, 10:10 p.m.)
> 
> 
> Review request for mesos and Anindya Sinha.
> 
> 
> Bugs: MESOS-7097
> https://issues.apache.org/jira/browse/MESOS-7097
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - With the new `register_agents` ACL, `permissive == false` would lead
>   to the agent unable to register unless explicitly allowed.
> 
> 
> Diffs
> -
> 
>   src/tests/script.cpp 3b68b845b06fe19acb8b08e1ff3dd0bec9117f05 
> 
> 
> Diff: https://reviews.apache.org/r/57730/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 57535: Applied RegisterAgent ACL to the master.

[Jiang Yan Xu]

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

(Updated April 20, 2017, 9:53 p.m.)


Review request for mesos, Adam B, Anindya Sinha, Alexander Rojas, Benjamin 
Mahler, Greg Mann, and Vinod Kone.


Changes
---

Rebase. Please review.


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


Repository: mesos


Description
---

Applied RegisterAgent ACL to the master.


Diffs (updated)
-

  src/master/master.hpp d537933d0b467a6f9996951c601b31338bb9d034 
  src/master/master.cpp d1cdc35a066e190ef8e0bd788e07e63b92f7fce7 
  src/tests/master_authorization_tests.cpp 
1a0285a3f345ef21a8256d4123d8bb684f538da4 


Diff: https://reviews.apache.org/r/57535/diff/7/

Changes: https://reviews.apache.org/r/57535/diff/6-7/


Testing
---

make check.

The tests added here cover some basic scenarios, I have more tests but will add 
them when MESOS-7244 is fixed.


Thanks,

Jiang Yan Xu

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

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

(Updated April 21, 2017, 4:32 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Tweak comment wording.


Repository: mesos


Description
---

This commit replaces the sorter's flat list of clients with a tree; the
tree represents the hierarchical relationship between sorter clients.
Each node in the tree contains a vector of pointers to child nodes. The
tree might contain nodes that do not correspond directly to sorter
clients. For example, adding clients "a/b" and "c/d" results in the
following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each active
client in the sorter. This is implemented by ensuring that each sorter
client is associated with a leaf node in the tree (i.e., internal nodes
are not returned by `sort`). Note that it is possible for a leaf node to
be transformed into an internal node by a subsequent insertion; to
handle this case, we "implicitly" create an extra child node, which
maintains the invariant that each client is associated with a leaf
node. For example, if the client "a/b/x" is added to the tree above, the
result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x". The "." node is called a "virtual leaf node".

This commit also introduces a new approach to sorting: rather than
keeping a `std::set` of sorter clients, we now keep a tree of
`std::vector`, which is sorted explicitly via `std::sort` when
necessary. The previous implementation tried to optimize the sorting
process by updating the sort order incrementally when a single sorter
client was updated; this commit removes that optimization, and instead
re-sorts the entire tree whenever a change is made that might alter the
sort order. Re-introducing a version of this optimization would make
sense in the future (MESOS-7390), but benchmarking suggests that this
commit results in a net improvement in sorter performance for
non-hierarchical clients, anyway. The performance improvement is likely
due to the introduction of a secondary hashmap that allows the leaf node
associated with a client name to be found efficiently; the previous
implementation required a linear scan of a `std::set`.


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_allocator_tests.cpp 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
  src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 


Diff: https://reviews.apache.org/r/57254/diff/24/

Changes: https://reviews.apache.org/r/57254/diff/23-24/


Testing
---


Thanks,

Neil Conway

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

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

(Updated April 21, 2017, 3:55 a.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Address review comments: braces, `add` changes.


Repository: mesos


Description
---

This commit replaces the sorter's flat list of clients with a tree; the
tree represents the hierarchical relationship between sorter clients.
Each node in the tree contains a vector of pointers to child nodes. The
tree might contain nodes that do not correspond directly to sorter
clients. For example, adding clients "a/b" and "c/d" results in the
following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each active
client in the sorter. This is implemented by ensuring that each sorter
client is associated with a leaf node in the tree (i.e., internal nodes
are not returned by `sort`). Note that it is possible for a leaf node to
be transformed into an internal node by a subsequent insertion; to
handle this case, we "implicitly" create an extra child node, which
maintains the invariant that each client is associated with a leaf
node. For example, if the client "a/b/x" is added to the tree above, the
result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x". The "." node is called a "virtual leaf node".

This commit also introduces a new approach to sorting: rather than
keeping a `std::set` of sorter clients, we now keep a tree of
`std::vector`, which is sorted explicitly via `std::sort` when
necessary. The previous implementation tried to optimize the sorting
process by updating the sort order incrementally when a single sorter
client was updated; this commit removes that optimization, and instead
re-sorts the entire tree whenever a change is made that might alter the
sort order. Re-introducing a version of this optimization would make
sense in the future (MESOS-7390), but benchmarking suggests that this
commit results in a net improvement in sorter performance for
non-hierarchical clients, anyway. The performance improvement is likely
due to the introduction of a secondary hashmap that allows the leaf node
associated with a client name to be found efficiently; the previous
implementation required a linear scan of a `std::set`.


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_allocator_tests.cpp 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
  src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 


Diff: https://reviews.apache.org/r/57254/diff/23/

Changes: https://reviews.apache.org/r/57254/diff/22-23/


Testing
---


Thanks,

Neil Conway

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Line 263 (original), 380 (patched)
> > 
> >
> > `s/DRFComparator/compare/`?
> > 
> > We typically name the parameters `left` and `right`
> 
> Neil Conway wrote:
> Personally I like `DRFComparator` more: it makes it clear that the 
> function is comparing the two nodes according to DRF.
> 
> I renamed the parameters.
> 
> Michael Park wrote:
> `compareDRF`? I guess I was more-so saying that it's no longer an object.

Done.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 117-125 (patched)
> > 
> >
> > I think this would be cleaner to say:
> > ```cpp
> > auto iter = std::find_if(
> > current->children.begin(),
> > current->children.end(),
> > [&](const Node* child) { return child->name == element; });
> > 
> > if (iter != current->children.end()) {
> >   current = *iter;
> >   continue;
> > }
> > 
> > /* ... */
> > ```
> 
> Neil Conway wrote:
> Happy to discuss further, but this seems more difficult to read, 
> personally. I feel like we usually use an explicit `foreach` loop in similar 
> situations elsewhere...

Per discussion, I revised the existing loop to remove the `found` variable.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 75-77 (original), 143-172 (patched)
> > 
> >
> > Kind of seems like it'd be simpler to just modify `current`.
> > 
> > ```
> > parent->removeChild(current);
> > 
> > // Create a node under `parent`. This internal node will become the 
> > new parent.
> > Node* internal = new Node(current->name, parent);
> > parent->addChild(internal);
> > internal->allocation = current->allocation;
> > CHECK_EQ(path, internal->path);
> > 
> > // Update `current` to become the virtual leaf node.
> > current->name = ".";
> > current->parent = internal;
> > internal->addChild(current);
> > current->path = strings::join("/", parent->path, current->name);
> > 
> > current = internal;
> > ```
> 
> Neil Conway wrote:
> Happy to discuss further, but personally I prefer the current coding 
> because it seems safer to avoid mutating an existing tree node "in place". 
> e.g., if there were other pointers to `current` that should _not_ be updated 
> to point to the virtual leaf node, they will now be dangling pointers (i.e., 
> clearly wrong) rather than now silently pointing at the virtual leaf node.

Per discussion, I changed this to use the approach suggested above.


- Neil


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


On April 20, 2017, 8:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated April 20, 2017, 8:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree; the
> tree represents the hierarchical relationship between sorter clients.
> Each node in the tree contains a vector of pointers to child nodes. The
> tree might contain nodes that do not correspond directly to sorter
> clients. For example, adding clients "a/b" and "c/d" results in the
> following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each active
> client in the sorter. This is implemented by ensuring that each sorter
> client is associated with a leaf node in the tree (i.e., internal nodes
> are not returned by `sort`). Note that it is possible for a leaf node to
> be transformed into an internal node by a subsequent insertion; to
> handle this case, we "implicitly" create an extra child node, which
> maintains the invariant that each client is associated with a leaf
> node. For example, if the client "a/b/x" is added to the tree above, the
> result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at 

Re: Review Request 53840: Metric in the allocator to track latency between allocation cycles.

[James Peach]

> On April 20, 2017, 6:04 p.m., James Peach wrote:
> > If I understand this correctly, this is publishing a time series of the 
> > idle time between allocator runs. Is that correct? If so, what insight does 
> > this metric give and how should operators interpret it? Why is a simple 
> > counter of allocator runs not sufficient?
> 
> Anindya Sinha wrote:
> So as to determine how frequently (or infrequently) allocation cycles are 
> being run by the allocator. The counters provide the current count of 
> allocation runs.

As we discussed with @xujyan, this is intended to measure the queueing delay 
for allocation dispatches. I'm still not convinced that this is a helpful 
metric. As implemented, this appears to measure the time between allocation 
intervals. I guess you would start the timer in 
`HierarchicalAllocatorProcess::allocate` and stop it in 
`HierarchicalAllocatorProcess::_allocate`.


- James


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


On April 19, 2017, 8:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53840/
> ---
> 
> (Updated April 19, 2017, 8:51 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a metric 'allocator/mesos/allocation_run_interval_ms' which
> tracks the latency between consecutive allocation cycles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0059ccead90f32491591990c791e7fa905a864b7 
>   src/master/allocator/mesos/metrics.hpp 
> ce486eb079dc44bf0bbfad8668426c7ae87c97b3 
>   src/master/allocator/mesos/metrics.cpp 
> ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
> 
> 
> Diff: https://reviews.apache.org/r/53840/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58357: Support more test frameworks in test-upgrade script.

[Greg Mann]

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



Tested this out; works great!! Just a couple small things below.


support/test-upgrade.py
Lines 298-302 (patched)


Would it be possible to allow the user to exclude more than one at a time? 
I would also be find with the user specifying explicitly a list of the 
frameworks they _do_ want to run.



support/test-upgrade.py
Lines 325-326 (patched)


Could you add an extra newline or two of logging at the end of each 
iteration? It's a bit hard to read the logging output since there are no 
newlines between the end of each framework run.


- Greg Mann


On April 19, 2017, 4:28 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58357/
> ---
> 
> (Updated April 19, 2017, 4:28 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Greg Mann.
> 
> 
> Bugs: MESOS-7228
> https://issues.apache.org/jira/browse/MESOS-7228
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch added support to java and python based test framework in
> `test-upgrade.py` script.
> 
> 
> Diffs
> -
> 
>   support/test-upgrade.py 84df21dd6e63653e1a18e700ef904aa3a04b2b45 
> 
> 
> Diff: https://reviews.apache.org/r/58357/diff/3/
> 
> 
> Testing
> ---
> 
> Ran this on all three languages options for cpp, java and python.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53841: Added metrics for sorting of the sorters in the allocator.

[James Peach]

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



This also needs documentation added to the `docs/monitoring.md`.


src/master/allocator/sorter/drf/metrics.hpp
Lines 59 (patched)


Why is this necessary? Is there a reason that publishing the dominant 
resources and shares for quotas doesn't make sense?



src/master/allocator/sorter/drf/metrics.cpp
Lines 50 (patched)


Use `path::join()` here.



src/master/allocator/sorter/drf/metrics.cpp
Lines 64 (patched)


Can you do the initialization in the constructor?


- James Peach


On April 19, 2017, 8:52 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53841/
> ---
> 
> (Updated April 19, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Following metrics have been added:
> a) allocator/mesos/roles/sort_runs: Number of role level sorts.
> b) allocator/mesos/roles/sort_run: Latency in role level sorts.
> c) allocator/mesos/quotas/sort_runs: Number of quota level sorts.
> d) allocator/mesos/quotas/sort_run: Latency in quota level sorts.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 646f66e67d9c6b8c61fc6e6558a1db976a44c126 
>   src/master/allocator/sorter/drf/metrics.hpp 
> 61568cb520826ab59d675824b212e0d3deb63764 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
> 
> 
> Diff: https://reviews.apache.org/r/53841/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 53840: Metric in the allocator to track latency between allocation cycles.

[Anindya Sinha]

> On April 20, 2017, 6:04 p.m., James Peach wrote:
> > If I understand this correctly, this is publishing a time series of the 
> > idle time between allocator runs. Is that correct? If so, what insight does 
> > this metric give and how should operators interpret it? Why is a simple 
> > counter of allocator runs not sufficient?

So as to determine how frequently (or infrequently) allocation cycles are being 
run by the allocator. The counters provide the current count of allocation runs.


- Anindya


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


On April 19, 2017, 8:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53840/
> ---
> 
> (Updated April 19, 2017, 8:51 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a metric 'allocator/mesos/allocation_run_interval_ms' which
> tracks the latency between consecutive allocation cycles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0059ccead90f32491591990c791e7fa905a864b7 
>   src/master/allocator/mesos/metrics.hpp 
> ce486eb079dc44bf0bbfad8668426c7ae87c97b3 
>   src/master/allocator/mesos/metrics.cpp 
> ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
> 
> 
> Diff: https://reviews.apache.org/r/53840/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

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

(Updated April 20, 2017, 8:40 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Rename DRF comparison function, tweak commit description.


Repository: mesos


Description (updated)
---

This commit replaces the sorter's flat list of clients with a tree; the
tree represents the hierarchical relationship between sorter clients.
Each node in the tree contains a vector of pointers to child nodes. The
tree might contain nodes that do not correspond directly to sorter
clients. For example, adding clients "a/b" and "c/d" results in the
following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each active
client in the sorter. This is implemented by ensuring that each sorter
client is associated with a leaf node in the tree (i.e., internal nodes
are not returned by `sort`). Note that it is possible for a leaf node to
be transformed into an internal node by a subsequent insertion; to
handle this case, we "implicitly" create an extra child node, which
maintains the invariant that each client is associated with a leaf
node. For example, if the client "a/b/x" is added to the tree above, the
result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x". The "." node is called a "virtual leaf node".

This commit also introduces a new approach to sorting: rather than
keeping a `std::set` of sorter clients, we now keep a tree of
`std::vector`, which is sorted explicitly via `std::sort` when
necessary. The previous implementation tried to optimize the sorting
process by updating the sort order incrementally when a single sorter
client was updated; this commit removes that optimization, and instead
re-sorts the entire tree whenever a change is made that might alter the
sort order. Re-introducing a version of this optimization would make
sense in the future (MESOS-7390), but benchmarking suggests that this
commit results in a net improvement in sorter performance for
non-hierarchical clients, anyway. The performance improvement is likely
due to the introduction of a secondary hashmap that allows the leaf node
associated with a client name to be found efficiently; the previous
implementation required a linear scan of a `std::set`.


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_allocator_tests.cpp 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
  src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 


Diff: https://reviews.apache.org/r/57254/diff/22/

Changes: https://reviews.apache.org/r/57254/diff/21-22/


Testing
---


Thanks,

Neil Conway

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Michael Park]

> On April 20, 2017, 2:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Line 263 (original), 380 (patched)
> > 
> >
> > `s/DRFComparator/compare/`?
> > 
> > We typically name the parameters `left` and `right`
> 
> Neil Conway wrote:
> Personally I like `DRFComparator` more: it makes it clear that the 
> function is comparing the two nodes according to DRF.
> 
> I renamed the parameters.

`compareDRF`? I guess I was more-so saying that it's no longer an object.


- Michael


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


On April 20, 2017, 10:01 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated April 20, 2017, 10:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit replaces the sorter's flat list of clients with a tree; the
> tree represents the hierarchical relationship between sorter clients.
> Each node in the tree contains a vector of pointers to child nodes. The
> tree might contain nodes that do not correspond directly to sorter
> clients. For example, adding clients "a/b" and "c/d" results in the
> following tree:
> 
> root
>  -> a
>-> b
>  -> c
>-> d
> 
> The `sort` member function still only returns one result for each active
> client in the sorter. This is implemented by ensuring that each sorter
> client is associated with a leaf node in the tree. Note that it is
> possible for a leaf node to be transformed into an internal node by a
> subsequent insertion; to handle this case, we "implicitly" create an
> extra child node, which maintains the invariant that each client has a
> leaf node. For example, if the client "a/b/x" is added to the tree
> above, the result is:
> 
> root
>  -> a
>-> b
>  -> .
>  -> x
>  -> c
>-> d
> 
> The "." leaf node holds the allocation that has been made to the "a/b"
> client itself; the "a/b" node holds the sum of all the allocations that
> have been made to the subtree rooted at "a/b", which also includes
> "a/b/x".
> 
> This commit also introduces a new approach to sorting: rather than
> keeping an `std::set` of sorter clients, we now keep a tree of
> `std::vector`, which is sorted explicitly via `std::sort`. The previous
> implementation tried to optimize the sorting process by updating the
> sort order incrementally when a single sorter client was updated; this
> commit removes that optimization, and instead re-sorts the entire tree
> whenever the sort order is changed.
> 
> Re-introducing a version of this optimization would make sense in the
> future (MESOS-7390), but benchmarking suggests that this commit results
> in a net improvement in sorter performance anyway. The sorter perf
> improvement is likely due to the introduction of a secondary hashmap
> that allows the leaf node associated with a tree path to be find
> efficiently; the previous implementation required a linear scan of a
> `std::set`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/metrics.cpp 
> 15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 76329220e1115c1de7810fb69b943c78c078be59 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
>   src/master/allocator/sorter/sorter.hpp 
> b3029fcf7342406955760da53f1ae736769f308c 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
>   src/tests/master_allocator_tests.cpp 
> 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
>   src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 
> 
> 
> Diff: https://reviews.apache.org/r/57254/diff/21/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58580, 58512]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 20, 2017, 5:40 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 20, 2017, 5:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/4/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58487: Fix allocation quantities when shared resources are removed.

[Jiang Yan Xu]

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



Do we need this?

The method

```
virtual void update(
  const std::string& name,
  const SlaveID& slaveId,
  const Resources& oldAllocation,
  const Resources& newAllocation);
```

is not supposed to change the quantities (and we have another patch for adding 
a CHECK). We ought to be able to just assert that `oldAllocation` and 
`newAllocation` have the same quantity and then do not modify 
`allocations[name].scalarQuantities` or 
`allocations[name].totals[resource.name()]` at all. This is basically what the 
other does?

- Jiang Yan Xu


On April 18, 2017, 7:50 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58487/
> ---
> 
> (Updated April 18, 2017, 7:50 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7308
> https://issues.apache.org/jira/browse/MESOS-7308
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When shared resources are removed from the `allocations` in the sorter,
> we remove the scalar quantity only when the shared resource is actually
> removed (i.e. shared count goes down to 0). Similarly, we increase the
> scalar quantity only when this is a new shared resource that is being
> added to the `allocations`.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ed54680cecb637931fc344fbcf8fd3b14cc24295 
> 
> 
> Diff: https://reviews.apache.org/r/58487/diff/2/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58584: Disabled support for setting quota on nested roles.

[Neil Conway]

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

(Updated April 20, 2017, 6:18 p.m.)


Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.


Changes
---

Tweak description.


Summary (updated)
-

Disabled support for setting quota on nested roles.


Repository: mesos


Description (updated)
---

Correct support for quota on nested roles will require further work in
the allocator (see MESOS-7402 for details). For now, setting quota on
nested roles is disabled until MESOS-7402 can be fixed. This commit
disables any tests that rely on setting quota on nested roles; it also
adds a (disabled) test to cover the behavior that will be fixed as part
of MESOS-7402.


Diffs (updated)
-

  src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b 


Diff: https://reviews.apache.org/r/58584/diff/2/

Changes: https://reviews.apache.org/r/58584/diff/1-2/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58486: Fixed a race in `updateAllocation()` on DESTORY of a shared volume.

[Jiang Yan Xu]

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



I feel the following may be more clear and general at high level.

- In `_accept()` all operations are validated, for resource operations we call 
`updateAllocation` for each individual operation [like we used to 
do](https://github.com/apache/mesos/blob/e7fcc4fbc09122480c8f2549a8af099769e888c0/src/master/master.cpp#L3770).
 
- We fail `updateAllocate` if the operation cannot be applied but leave the 
allocator in a good state.
- Individual resource operations are sent to the agent only when 
`updateAllocate` succeeds, otherwise the operation is dropped with no bad side 
effects.

It'll be also great if we can come up with a test?


src/master/allocator/mesos/hierarchical.cpp
Lines 746-749 (original), 746-749 (patched)


Below here we can use the same mechanism as done in `updateAvailable`:

```
// In addition to asserting the operation can be applied to the offered
// resources above, here we also check if it can be applied to the 
agent's
// current allocated resources. We do this because certain operations
// require that they are applicable to the agent's allocated resources
// as a whole. One such example is that a shared persistent volume can
// only be destroyed by a framework when it is not allocated to other
// frameworks (see `Resources::apply` for details). The master already 
does
// best-effort validation but this check may still fail due to 
additional   
// `allocate` runs. In a race condition an `allocate` could have been
// enqueued by the allocator itself just before master's request to
// enqueue `updateAllocation` arrives to the allocator. This is similar
// to the siutation in `updateAvailable`, see the diagram there.
Try updatedAllocated = slave.allocated.apply(operation);
if (updatedAllocated.isError()) {
  return Failure(updatedAllocated.error());
}
```

We need to comment clearly on why we need this. Above I tried to state the 
general idea but also gave an concrete example but this is just a first draft.

For this to work we need to make sure `Resources::apply` fails in such a 
case: https://issues.apache.org/jira/browse/MESOS-7403

But now the failure would occur in the right place in this method (not 
applicable to the current allocated resources) and the error message returned 
by `Resources::apply` would be able to explain what exactly happened.



src/master/allocator/mesos/hierarchical.cpp
Lines 848-851 (original)


This being at the bottom of the method was intended to validate the 
post-condition. If this doesn't hold, ideally we shouldn't capture it here 
because we don't know how it has failed. We should capture the failure at the 
individual step and construct an error message/log that's explicit about it.



src/master/master.cpp
Line 4355 (original), 4355 (patched)


We should proably resurrect the previous 

```
void Master::apply(
Framework* framework,
Slave* slave,
const Offer::Operation& operation)
{
  CHECK_NOTNULL(framework);
  CHECK_NOTNULL(slave);

  allocator->updateAllocation(framework->id(), slave->id, {operation});

  _apply(slave, operation);
}
```


https://github.com/apache/mesos/blob/57a7e7d0283aa08455d6572ade75a11d914c6962/src/master/master.cpp#L5729-L5740

but make it async so we only call `_apply` when the result from 
`updateAllocation` is successful.

Then we call `updateAllocation` with just this single operation so when it 
fails it's due to this single operation.



src/master/master.cpp
Lines 4774-4775 (patched)


So if the failure has occurred, the allocation in the allocator is already 
wrong. e.g., it can have both a volume of 1MB (which didn't get removed) and 
the plain disk of 1MB (which got added assuming the volume was removed). 

Ideally when a failure happens it leaves the allocator in the previous good 
state but here we are making an attempt to asynchronously compensate that and 
to correct it. Many events can be executed on the allocator between the two.

While it's true that no new supposedly destroyed volumes can be offered out 
due to being removed from `slave.total` in the allocator, the allocator is 
itself running and the framework's incorrect allocation can cause issue, e.g., 
fairness, although there are probably more severe problems.


- Jiang Yan Xu


On April 18, 2017, 7:50 p.m., Anindya Sinha wrote:
> 

Review Request 58584: Disabled support for quota on nested roles.

[Neil Conway]

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

Review request for mesos, Benjamin Mahler, Jay Guo, and Michael Park.


Repository: mesos


Description
---

Correct support for quota on nested roles will require further work in
the allocator (see MESOS-7402 for details). For now, setting quota on
nested roles is disabled until MESOS-7402 can be fixed.


Diffs
-

  src/master/quota_handler.cpp 7ff43a048e17b9e9ac0ceed248f7b3fd56b007d6 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_quota_tests.cpp 1714ba13ea63bae05448d0898bf722ef472c672b 


Diff: https://reviews.apache.org/r/58584/diff/1/


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 57516: Updated CHANGELOG for hierarchical roles.

[Neil Conway]

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

(Updated April 20, 2017, 6:14 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Marked hroles as experimental; noted that setting quota for nested roles is not 
currently supported.


Repository: mesos


Description
---

Updated CHANGELOG for hierarchical roles.


Diffs (updated)
-

  CHANGELOG feaedf7426fe694e4ecf1ade82abca3bf69d1691 


Diff: https://reviews.apache.org/r/57516/diff/4/

Changes: https://reviews.apache.org/r/57516/diff/3-4/


Testing
---

No functional change.


Thanks,

Neil Conway

Re: Review Request 53840: Metric in the allocator to track latency between allocation cycles.

[James Peach]

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



If I understand this correctly, this is publishing a time series of the idle 
time between allocator runs. Is that correct? If so, what insight does this 
metric give and how should operators interpret it? Why is a simple counter of 
allocator runs not sufficient?

- James Peach


On April 19, 2017, 8:51 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53840/
> ---
> 
> (Updated April 19, 2017, 8:51 p.m.)
> 
> 
> Review request for mesos, James Peach and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6579
> https://issues.apache.org/jira/browse/MESOS-6579
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a metric 'allocator/mesos/allocation_run_interval_ms' which
> tracks the latency between consecutive allocation cycles.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0059ccead90f32491591990c791e7fa905a864b7 
>   src/master/allocator/mesos/metrics.hpp 
> ce486eb079dc44bf0bbfad8668426c7ae87c97b3 
>   src/master/allocator/mesos/metrics.cpp 
> ec987fe0ce8605bf9e7c0ccbe3ac384deaf53be1 
>   src/tests/hierarchical_allocator_tests.cpp 
> cdf1f15b7802439b28405ca8f6634ce83e886630 
> 
> 
> Diff: https://reviews.apache.org/r/53840/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

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

(Updated April 20, 2017, 5:40 p.m.)


Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

If the callback `on_headers_complete()` fails, the decoder's writer
object is `None()`. So, when the callback `on_message_complete()` is
called, we should not crash in that case.
Note that the `CHECK(decoder->writer)` is valid for the callback
`on_body()` is valid since this callback is called only on a success
in `on_headers_complete()` callback.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
c0efef571815752cc28e5a0dc7a07adc82bb31d5 


Diff: https://reviews.apache.org/r/58512/diff/4/

Changes: https://reviews.apache.org/r/58512/diff/3-4/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58580: Mark decoder as failed when http parsing failed for any reason.

[Anindya Sinha]

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

(Updated April 20, 2017, 5:40 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Addressed comments from https://reviews.apache.org/r/58512


Repository: mesos


Description
---

Mark decoder as failed when http parsing failed for any reason.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 


Diff: https://reviews.apache.org/r/58580/diff/2/

Changes: https://reviews.apache.org/r/58580/diff/1-2/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

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


Fix it, then Ship it!





3rdparty/libprocess/src/decoder.hpp
Line 441 (original)


Nit: Can you do these cleanups in the previous review?


- Anand Mazumdar


On April 20, 2017, 5:18 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 20, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/3/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58580: Mark decoder as failed when http parsing failed for any reason.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On April 20, 2017, 5:18 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58580/
> ---
> 
> (Updated April 20, 2017, 5:18 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mark decoder as failed when http parsing failed for any reason.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
> 
> 
> Diff: https://reviews.apache.org/r/58580/diff/1/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 58580: Mark decoder as failed when http parsing failed for any reason.

[Anindya Sinha]

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

Review request for mesos and Anand Mazumdar.


Repository: mesos


Description
---

Mark decoder as failed when http parsing failed for any reason.


Diffs
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 


Diff: https://reviews.apache.org/r/58580/diff/1/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

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

(Updated April 20, 2017, 5:18 p.m.)


Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.


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


Repository: mesos


Description
---

If the callback `on_headers_complete()` fails, the decoder's writer
object is `None()`. So, when the callback `on_message_complete()` is
called, we should not crash in that case.
Note that the `CHECK(decoder->writer)` is valid for the callback
`on_body()` is valid since this callback is called only on a success
in `on_headers_complete()` callback.


Diffs (updated)
-

  3rdparty/libprocess/src/decoder.hpp 4c779d42548958e610142438a57529ccb4478053 
  3rdparty/libprocess/src/tests/decoder_tests.cpp 
c0efef571815752cc28e5a0dc7a07adc82bb31d5 


Diff: https://reviews.apache.org/r/58512/diff/3/

Changes: https://reviews.apache.org/r/58512/diff/2-3/


Testing
---

All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anindya Sinha]

> On April 20, 2017, 4:06 p.m., Anand Mazumdar wrote:
> > 3rdparty/libprocess/src/decoder.hpp
> > Lines 208 (patched)
> > 
> >
> > hmm, these seem not related to the review i.e., avoiding the crash. Can 
> > you create a separate patch with these changes and mark this as dependent 
> > on it?
> > 
> > Good catch!

Separated these changes into https://reviews.apache.org/r/58580/ and made this 
RR depend on that one.


- Anindya


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


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58449: CMake: Bump minimum version to 3.7.0 on Windows.

[Andrew Schwartzmeyer]

> On April 18, 2017, 8:19 p.m., Jeff Coffler wrote:
> > Ship It!
> 
> Andrew Schwartzmeyer wrote:
> Hey, just wondering, why'd you give a "Ship It!" here, but then went back 
> to patches earlier in the chain and asked for test results (provided in the 
> patch previous to this)?
> 
> Jeff Coffler wrote:
> I'd like to chat with you about this. From my look with Review Board, I 
> now don't see "test results" here. I thought I did before.
> 
> I think I'm confused about how Review Board works. Stop by when you get 
> in and explain what I'm missing, thanks!

Sure thing :)


- Andrew


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


On April 14, 2017, 2:10 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58449/
> ---
> 
> (Updated April 14, 2017, 2:10 a.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SOURCE_SUBDIR` command to `ExternalProject_Add` was added in CMake
> 3.7.0, and is necessary to most cleanly build an external CMake built
> project where the `CMakeLists.txt` is in a subfolder of the project.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ea529ec2d5c2b9be4f19c67c2033c3f4b9073c1f 
> 
> 
> Diff: https://reviews.apache.org/r/58449/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58448: Windows: Updated ZooKeeper to use CMake.

[Andrew Schwartzmeyer]

> On April 18, 2017, 8:30 p.m., Jeff Coffler wrote:
> > Big change, but important for us.
> > 
> > I'd like you to comment on using cmake for zookeeper, specifically in terms 
> > of rolling that back to Zookeeper themselves for them to maintain it.
> > 
> > Have you looked at contributing this to the Zookeeper folks for them to use 
> > and maintain? What was their response?
> > 
> > This feels like something that is easily broken (mentioning specific 
> > sources, etc) if they aren't maintaining it, and I'd rather not own this 
> > for life. If the Zookeeper folks don't want to maintain this, what do they 
> > say about VS 2017 support? Even if it's not cmake, but a hand-generated 
> > solution for V/S, at least they maintain it, not us.
> 
> Andrew Schwartzmeyer wrote:
> Ah, sorry, I moved bugs around. Just attached the associated ZooKeeper 
> bug with what you're looking for.
> 
> Jeff Coffler wrote:
> But my question remains: Will the Zookeeper folks take this and own it, 
> or is it ours for live? More importantly, what do the Zookeeper folks have to 
> say about VS 2017 support? Do they have an answer, or are they embracing your 
> change as the answer?
> 
> This really comes down to: Who owns this with time? Us (if we take a new 
> version of Zookeeper), or the Zookeeper team?

They are [reviewing the 
patch](https://issues.apache.org/jira/browse/ZOOKEEPER-2756) and have requested 
a PR over a patch file.

We own our patches, this simply replaces the current patches to ZooKeeper we 
already own. Theoretically we could update to a newer ZooKeeper if they release 
one with my changes included, but it doesn't block us.


- Andrew


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


On April 19, 2017, 6:20 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58448/
> ---
> 
> (Updated April 19, 2017, 6:20 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: ZOOKEEPER-2756
> https://issues.apache.org/jira/browse/ZOOKEEPER-2756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This unblocks us from building exclusively with VS 2017. The previous
> patch to ZooKeeper only added VS 2015 support. This patch replaces it
> with a CMake build system that will generate whichever solution we need
> for Windows (and can replace the Autotools system on Linux).
> 
> We're updating to 3.5.2-alpha as the existing 3.5.1 rebundle was a
> source tarball, and so missing the necessary generated files. The most
> currently used version was based off a random commit. 3.5.2-alpha is the
> latest 3.5.x release of ZooKeeper (3.5.x itself is alpha, 3.5.2 is
> semi-stable, in comparison to 3.5.3 which is in RC).
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt bb61ef0514fb164f35b34bb6be1bbebb4d1a1861 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> c60652688a23f8628f133b7890ff39e38fc8ae94 
>   3rdparty/cmake/Versions.cmake 912726351ff744dd839b8d1c8d64dcc373d879be 
>   3rdparty/zookeeper-3.5.2-alpha.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58448/diff/1/
> 
> 
> Testing
> ---
> 
> cmake and make check on Linux
> stout-tests, libprocess-tests, and mesos-tests on Windows: all tests passed. 
> (Note: my machine updated to the Creators Update today, disabling long path 
> support; the previous way to enable it doesn't seem to work, so I re-ran the 
> failed tests on a machine that hadn't updated and still had long path 
> support, they passed).
> 
> Also tested the build on a clean machine with nothing but the VS 2017 build 
> tools (no IDE, no prior VS installations); mesos-tests builds no problem.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.hpp
> > Line 263 (original), 380 (patched)
> > 
> >
> > `s/DRFComparator/compare/`?
> > 
> > We typically name the parameters `left` and `right`

Personally I like `DRFComparator` more: it makes it clear that the function is 
comparing the two nodes according to DRF.

I renamed the parameters.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 74 (original), 108-116 (patched)
> > 
> >
> > There are `nameElements` and `element` here rather than 
> > `elements/element` or `nameElements/nameElement`. I think we also used 
> > `components/component` in `quotaHandler`. Maybe it'd be better to stick 
> > with that naming scheme?

I renamed `name` to `clientPath` and used `pathElements`/`element`, which seems 
OK to me.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 117-125 (patched)
> > 
> >
> > I think this would be cleaner to say:
> > ```cpp
> > auto iter = std::find_if(
> > current->children.begin(),
> > current->children.end(),
> > [&](const Node* child) { return child->name == element; });
> > 
> > if (iter != current->children.end()) {
> >   current = *iter;
> >   continue;
> > }
> > 
> > /* ... */
> > ```

Happy to discuss further, but this seems more difficult to read, personally. I 
feel like we usually use an explicit `foreach` loop in similar situations 
elsewhere...


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 128 (patched)
> > 
> >
> > I think the "// We didn't find `element`, so add a new child to 
> > `current`.` is better placed after the "leaf -> internal" if block, where 
> > we actually add the new child.

Personally I think it is important to have a comment at the start of the `if 
(!found) { ... }`, to explain what the entire block is doing. I added a second 
comment down below, though.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Lines 75-77 (original), 143-172 (patched)
> > 
> >
> > Kind of seems like it'd be simpler to just modify `current`.
> > 
> > ```
> > parent->removeChild(current);
> > 
> > // Create a node under `parent`. This internal node will become the 
> > new parent.
> > Node* internal = new Node(current->name, parent);
> > parent->addChild(internal);
> > internal->allocation = current->allocation;
> > CHECK_EQ(path, internal->path);
> > 
> > // Update `current` to become the virtual leaf node.
> > current->name = ".";
> > current->parent = internal;
> > internal->addChild(current);
> > current->path = strings::join("/", parent->path, current->name);
> > 
> > current = internal;
> > ```

Happy to discuss further, but personally I prefer the current coding because it 
seems safer to avoid mutating an existing tree node "in place". e.g., if there 
were other pointers to `current` that should _not_ be updated to point to the 
virtual leaf node, they will now be dangling pointers (i.e., clearly wrong) 
rather than now silently pointing at the virtual leaf node.


> On April 20, 2017, 9:35 a.m., Michael Park wrote:
> > src/master/allocator/sorter/drf/sorter.cpp
> > Line 155 (original), 328 (patched)
> > 
> >
> > ```
> > for (Node* current = CHECK_NOTNULL(find(name));
> >  current != root;
> >  current = CHECK_NOTNULL(current->parent))
> > ```
> > 
> > Other similar situations below.

IMO the current coding is more readable, but happy to discuss further if you 
disagree.


- Neil


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


On April 20, 2017, 12:10 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated April 20, 2017, 12:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Neil Conway]

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

(Updated April 20, 2017, 5:01 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.


Changes
---

Address review comments.


Repository: mesos


Description
---

This commit replaces the sorter's flat list of clients with a tree; the
tree represents the hierarchical relationship between sorter clients.
Each node in the tree contains a vector of pointers to child nodes. The
tree might contain nodes that do not correspond directly to sorter
clients. For example, adding clients "a/b" and "c/d" results in the
following tree:

root
 -> a
   -> b
 -> c
   -> d

The `sort` member function still only returns one result for each active
client in the sorter. This is implemented by ensuring that each sorter
client is associated with a leaf node in the tree. Note that it is
possible for a leaf node to be transformed into an internal node by a
subsequent insertion; to handle this case, we "implicitly" create an
extra child node, which maintains the invariant that each client has a
leaf node. For example, if the client "a/b/x" is added to the tree
above, the result is:

root
 -> a
   -> b
 -> .
 -> x
 -> c
   -> d

The "." leaf node holds the allocation that has been made to the "a/b"
client itself; the "a/b" node holds the sum of all the allocations that
have been made to the subtree rooted at "a/b", which also includes
"a/b/x".

This commit also introduces a new approach to sorting: rather than
keeping an `std::set` of sorter clients, we now keep a tree of
`std::vector`, which is sorted explicitly via `std::sort`. The previous
implementation tried to optimize the sorting process by updating the
sort order incrementally when a single sorter client was updated; this
commit removes that optimization, and instead re-sorts the entire tree
whenever the sort order is changed.

Re-introducing a version of this optimization would make sense in the
future (MESOS-7390), but benchmarking suggests that this commit results
in a net improvement in sorter performance anyway. The sorter perf
improvement is likely due to the introduction of a secondary hashmap
that allows the leaf node associated with a tree path to be find
efficiently; the previous implementation required a linear scan of a
`std::set`.


Diffs (updated)
-

  src/master/allocator/sorter/drf/metrics.cpp 
15aab32db5ca1a7a14080e9bbb7c65283be3ec20 
  src/master/allocator/sorter/drf/sorter.hpp 
76329220e1115c1de7810fb69b943c78c078be59 
  src/master/allocator/sorter/drf/sorter.cpp 
ed54680cecb637931fc344fbcf8fd3b14cc24295 
  src/master/allocator/sorter/sorter.hpp 
b3029fcf7342406955760da53f1ae736769f308c 
  src/tests/hierarchical_allocator_tests.cpp 
33e7b455f8664858eb4f03727b076a10c80cd6e0 
  src/tests/master_allocator_tests.cpp 119e318f8a01d50e8dae5c30cf5fa6a017c3c625 
  src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 


Diff: https://reviews.apache.org/r/57254/diff/21/

Changes: https://reviews.apache.org/r/57254/diff/20-21/


Testing
---


Thanks,

Neil Conway

Re: Review Request 58508: Introduced method `Resources::inheritable` to aggregate reservations.

[Jay Guo]

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

(Updated April 21, 2017, 12:18 a.m.)


Review request for mesos and Michael Park.


Changes
---

address comments from mcypark.


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


Repository: mesos


Description
---

A reservation is inheritable by 'role' iff:
 - it is reserved to ancestor of 'role' in hierarchy, OR
 - it is reserved to 'role' itself.

This is a helper function for reservation delegate, where reserved
resources can be 'delegated' downwards in the hierarchy.


Diffs (updated)
-

  include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
  src/common/resources.cpp 77bac0c4390da905016a942991b14a79877f8455 
  src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be 


Diff: https://reviews.apache.org/r/58508/diff/2/

Changes: https://reviews.apache.org/r/58508/diff/1-2/


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 58512: Avoid a crash in HTTP decoder.

[Anand Mazumdar]

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




3rdparty/libprocess/src/decoder.hpp
Lines 208 (patched)


hmm, these seem not related to the review i.e., avoiding the crash. Can you 
create a separate patch with these changes and mark this as dependent on it?

Good catch!



3rdparty/libprocess/src/decoder.hpp
Lines 715 (patched)


set `decoder->failed=true` here? If it's already set earlier, add an 
explicit invariant check here before this line?



3rdparty/libprocess/src/decoder.hpp
Lines 1017 (patched)


Ditto as above.


- Anand Mazumdar


On April 19, 2017, 4:02 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58512/
> ---
> 
> (Updated April 19, 2017, 4:02 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, James Peach, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7400
> https://issues.apache.org/jira/browse/MESOS-7400
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the callback `on_headers_complete()` fails, the decoder's writer
> object is `None()`. So, when the callback `on_message_complete()` is
> called, we should not crash in that case.
> Note that the `CHECK(decoder->writer)` is valid for the callback
> `on_body()` is valid since this callback is called only on a success
> in `on_headers_complete()` callback.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp 
> 4c779d42548958e610142438a57529ccb4478053 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 
> c0efef571815752cc28e5a0dc7a07adc82bb31d5 
> 
> 
> Diff: https://reviews.apache.org/r/58512/diff/2/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 58287: Print corresponding address when socket shutdown.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58287]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 20, 2017, 7:34 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58287/
> ---
> 
> (Updated April 20, 2017, 7:34 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Print corresponding address when socket shutdown.
> Default just print socket 'fd',it's not convenient
> to find corresponding address.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 92efa91 
> 
> 
> Diff: https://reviews.apache.org/r/58287/diff/5/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> File Attachments
> 
> 
> process.patch
>   
> https://reviews.apache.org/media/uploaded/files/2017/04/13/d5736720-1262-484a-8efd-6e1eeada944d__process.patch
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 58449: CMake: Bump minimum version to 3.7.0 on Windows.

[Jeff Coffler]

> On April 18, 2017, 8:19 p.m., Jeff Coffler wrote:
> > Ship It!
> 
> Andrew Schwartzmeyer wrote:
> Hey, just wondering, why'd you give a "Ship It!" here, but then went back 
> to patches earlier in the chain and asked for test results (provided in the 
> patch previous to this)?

I'd like to chat with you about this. From my look with Review Board, I now 
don't see "test results" here. I thought I did before.

I think I'm confused about how Review Board works. Stop by when you get in and 
explain what I'm missing, thanks!


- Jeff


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


On April 14, 2017, 2:10 a.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58449/
> ---
> 
> (Updated April 14, 2017, 2:10 a.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SOURCE_SUBDIR` command to `ExternalProject_Add` was added in CMake
> 3.7.0, and is necessary to most cleanly build an external CMake built
> project where the `CMakeLists.txt` is in a subfolder of the project.
> 
> 
> Diffs
> -
> 
>   CMakeLists.txt ea529ec2d5c2b9be4f19c67c2033c3f4b9073c1f 
> 
> 
> Diff: https://reviews.apache.org/r/58449/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58448: Windows: Updated ZooKeeper to use CMake.

[Jeff Coffler]

> On April 18, 2017, 8:30 p.m., Jeff Coffler wrote:
> > Big change, but important for us.
> > 
> > I'd like you to comment on using cmake for zookeeper, specifically in terms 
> > of rolling that back to Zookeeper themselves for them to maintain it.
> > 
> > Have you looked at contributing this to the Zookeeper folks for them to use 
> > and maintain? What was their response?
> > 
> > This feels like something that is easily broken (mentioning specific 
> > sources, etc) if they aren't maintaining it, and I'd rather not own this 
> > for life. If the Zookeeper folks don't want to maintain this, what do they 
> > say about VS 2017 support? Even if it's not cmake, but a hand-generated 
> > solution for V/S, at least they maintain it, not us.
> 
> Andrew Schwartzmeyer wrote:
> Ah, sorry, I moved bugs around. Just attached the associated ZooKeeper 
> bug with what you're looking for.

But my question remains: Will the Zookeeper folks take this and own it, or is 
it ours for live? More importantly, what do the Zookeeper folks have to say 
about VS 2017 support? Do they have an answer, or are they embracing your 
change as the answer?

This really comes down to: Who owns this with time? Us (if we take a new 
version of Zookeeper), or the Zookeeper team?


- Jeff


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


On April 19, 2017, 6:20 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58448/
> ---
> 
> (Updated April 19, 2017, 6:20 p.m.)
> 
> 
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, and Li Li.
> 
> 
> Bugs: ZOOKEEPER-2756
> https://issues.apache.org/jira/browse/ZOOKEEPER-2756
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This unblocks us from building exclusively with VS 2017. The previous
> patch to ZooKeeper only added VS 2015 support. This patch replaces it
> with a CMake build system that will generate whichever solution we need
> for Windows (and can replace the Autotools system on Linux).
> 
> We're updating to 3.5.2-alpha as the existing 3.5.1 rebundle was a
> source tarball, and so missing the necessary generated files. The most
> currently used version was based off a random commit. 3.5.2-alpha is the
> latest 3.5.x release of ZooKeeper (3.5.x itself is alpha, 3.5.2 is
> semi-stable, in comparison to 3.5.3 which is in RC).
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt bb61ef0514fb164f35b34bb6be1bbebb4d1a1861 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> c60652688a23f8628f133b7890ff39e38fc8ae94 
>   3rdparty/cmake/Versions.cmake 912726351ff744dd839b8d1c8d64dcc373d879be 
>   3rdparty/zookeeper-3.5.2-alpha.patch PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/58448/diff/1/
> 
> 
> Testing
> ---
> 
> cmake and make check on Linux
> stout-tests, libprocess-tests, and mesos-tests on Windows: all tests passed. 
> (Note: my machine updated to the Creators Update today, disabling long path 
> support; the previous way to enable it doesn't seem to work, so I re-ran the 
> failed tests on a machine that hadn't updated and still had long path 
> support, they passed).
> 
> Also tested the build on a clean machine with nothing but the VS 2017 build 
> tools (no IDE, no prior VS installations); mesos-tests builds no problem.
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 58561: Enabling automatic protobuf flag parsing (for mesos).

[Mesos Reviewbot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 20, 2017, 10:21 a.m., Zhongbo Tian wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58561/
> ---
> 
> (Updated April 20, 2017, 10:21 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabling automatic protobuf flag parsing (for mesos).
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp e90738a91161e26a48a6e381765e631492294641 
> 
> 
> Diff: https://reviews.apache.org/r/58561/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhongbo Tian
> 
>

Re: Review Request 58557: Print failure reason when socket accept failed.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58557]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On April 20, 2017, 9:35 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58557/
> ---
> 
> (Updated April 20, 2017, 9:35 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Print failure reason when socket accept failed.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 92efa91 
> 
> 
> Diff: https://reviews.apache.org/r/58557/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/checks/checker.cpp
Lines 1104 (patched)


I think you're missing a space after "port".


- Gastón Kleiman


On April 19, 2017, 3:16 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58196/
> ---
> 
> (Updated April 19, 2017, 3:16 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp fec30a22494c2684377c579b601cac1f4e909608 
>   src/checks/checker.cpp cf7a086ead4413083ea1e28112f1f22dc18f0a89 
>   src/launcher/default_executor.cpp d003c1b307c0c258fd82028ea7d932d92653e746 
>   src/launcher/executor.cpp d14fbfb65d1cc44c954c8cdbbf36d7ddf6ede19e 
>   src/tests/check_tests.cpp 79ba5eb38b6e7338392fb17ad39f6cd250f87d88 
> 
> 
> Diff: https://reviews.apache.org/r/58196/diff/3/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various linux variants.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58287: Print corresponding address when socket shutdown.

[Andy Pang]

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

(Updated 四月 20, 2017, 11:34 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

Print corresponding address when socket shutdown.
Default just print socket 'fd',it's not convenient
to find corresponding address.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 92efa91 


Diff: https://reviews.apache.org/r/58287/diff/5/

Changes: https://reviews.apache.org/r/58287/diff/4-5/


Testing
---

make check


File Attachments


process.patch
  
https://reviews.apache.org/media/uploaded/files/2017/04/13/d5736720-1262-484a-8efd-6e1eeada944d__process.patch


Thanks,

Andy Pang

Re: Review Request 58194: Hardened HTTP check tests.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On April 19, 2017, 3:16 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58194/
> ---
> 
> (Updated April 19, 2017, 3:16 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduce 1s delay to ensure the task (HTTP server) has enough time
> to start and start serving request.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp 79ba5eb38b6e7338392fb17ad39f6cd250f87d88 
> 
> 
> Diff: https://reviews.apache.org/r/58194/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 58561: Enabling automatic protobuf flag parsing (for mesos).

[Zhongbo Tian]

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

Review request for mesos.


Repository: mesos


Description
---

Enabling automatic protobuf flag parsing (for mesos).


Diffs
-

  src/common/parse.hpp e90738a91161e26a48a6e381765e631492294641 


Diff: https://reviews.apache.org/r/58561/diff/1/


Testing
---


Thanks,

Zhongbo Tian

Review Request 58560: Enabling automatic protobuf flag parsing (for stout).

[Zhongbo Tian]

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

Review request for mesos.


Repository: mesos


Description
---

Enabling automatic protobuf flag parsing (for stout).


Diffs
-

  3rdparty/stout/include/stout/flags/parse.hpp 
65edd86372596c2107e9f29cf27301e025e6620e 


Diff: https://reviews.apache.org/r/58560/diff/1/


Testing
---


Thanks,

Zhongbo Tian

Re: Review Request 58508: Introduced method `Resources::inheritable` to aggregate reservations.

[Michael Park]

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




src/common/resources.cpp
Lines 909 (patched)


This would incorrectly declare `foo/bar` to be inheritable by `foo/ba`, 
right? I think we need to `tokenize` and check for prefix of the result?


- Michael Park


On April 18, 2017, 8:43 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58508/
> ---
> 
> (Updated April 18, 2017, 8:43 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A reservation is inheritable by 'role' iff:
>  - it is reserved to ancestor of 'role' in hierarchy, OR
>  - it is reserved to 'role' itself.
> 
> This is a helper function for reservation delegate, where reserved
> resources can be 'delegated' downwards in the hierarchy.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
>   src/common/resources.cpp 77bac0c4390da905016a942991b14a79877f8455 
>   src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be 
> 
> 
> Diff: https://reviews.apache.org/r/58508/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 58557: Print failure reason when socket accept failed.

[Andy Pang]

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

Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

Print failure reason when socket accept failed.


Diffs
-

  3rdparty/libprocess/src/process.cpp 92efa91 


Diff: https://reviews.apache.org/r/58557/diff/1/


Testing
---

make check


Thanks,

Andy Pang

Re: Review Request 57254: Added support for hierarchical roles to DRFSorter.

[Michael Park]

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



Overall, it's looking good!

I think there are a few places where `Node*` should be marked `const Node*`.

One of the things that I would like to see if the cleaning up of some the 
terminology being used.
For example, I think the use of `name` is used for both `clientPath` as well as 
the last part of the "full path".
`logicalPath` seems exactly `clientPath`, so it seems that we could consolidate 
that as well.

I decided to drop the efforts to pull out `Node::createVirtualNode()` because 
it ended up
needing separate cases anyway. (for creating one off of an internal node vs a 
leaf node.)


src/master/allocator/sorter/drf/sorter.hpp
Lines 57 (patched)


It seems that we should update these `name`s to be `clientPath`s?



src/master/allocator/sorter/drf/sorter.hpp
Lines 112 (patched)


Let's also remove this debugging utility.



src/master/allocator/sorter/drf/sorter.hpp
Line 44 (original), 198-201 (patched)


```
  Node(const std::string& _name, Node* _parent)
: name(_name), share(0), active(false), parent(_parent)
  {
```



src/master/allocator/sorter/drf/sorter.hpp
Lines 246 (patched)


Can we refer to this as `clientPath`?



src/master/allocator/sorter/drf/sorter.hpp
Lines 248-249 (patched)


`return CHECK_NOTNULL(parent)->path;`



src/master/allocator/sorter/drf/sorter.hpp
Line 263 (original), 380 (patched)


`s/DRFComparator/compare/`?

We typically name the parameters `left` and `right`



src/master/allocator/sorter/drf/sorter.cpp
Lines 46-56 (original), 47-77 (patched)


We generally don't have functions like this. Let's get rid of them please.



src/master/allocator/sorter/drf/sorter.cpp
Line 74 (original), 108-116 (patched)


There are `nameElements` and `element` here rather than `elements/element` 
or `nameElements/nameElement`. I think we also used `components/component` in 
`quotaHandler`. Maybe it'd be better to stick with that naming scheme?



src/master/allocator/sorter/drf/sorter.cpp
Lines 117-125 (patched)


I think this would be cleaner to say:
```cpp
auto iter = std::find_if(
current->children.begin(),
current->children.end(),
[&](const Node* child) { return child->name == element; });

if (iter != current->children.end()) {
  current = *iter;
  continue;
}

/* ... */
```



src/master/allocator/sorter/drf/sorter.cpp
Lines 128 (patched)


I think the "// We didn't find `element`, so add a new child to `current`.` 
is better placed after the "leaf -> internal" if block, where we actually add 
the new child.



src/master/allocator/sorter/drf/sorter.cpp
Lines 75-77 (original), 143-172 (patched)


Kind of seems like it'd be simpler to just modify `current`.

```
parent->removeChild(current);

// Create a node under `parent`. This internal node will become the new 
parent.
Node* internal = new Node(current->name, parent);
parent->addChild(internal);
internal->allocation = current->allocation;
CHECK_EQ(path, internal->path);

// Update `current` to become the virtual leaf node.
current->name = ".";
current->parent = internal;
internal->addChild(current);
current->path = strings::join("/", parent->path, current->name);

current = internal;
```



src/master/allocator/sorter/drf/sorter.cpp
Line 155 (original), 328 (patched)


```
for (Node* current = CHECK_NOTNULL(find(name));
 current != root;
 current = CHECK_NOTNULL(current->parent))
```

Other similar situations below.


- Michael Park


On April 19, 2017, 5:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57254/
> ---
> 
> (Updated April 19, 2017, 5:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit 

Re: Review Request 58287: Print corresponding address when socket shutdown.

[Andy Pang]

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

(Updated 四月 20, 2017, 9:09 a.m.)


Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

Print corresponding address when socket shutdown.
Default just print socket 'fd',it's not convenient
to find corresponding address.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 92efa91 


Diff: https://reviews.apache.org/r/58287/diff/4/

Changes: https://reviews.apache.org/r/58287/diff/3-4/


Testing
---

make check


File Attachments


process.patch
  
https://reviews.apache.org/media/uploaded/files/2017/04/13/d5736720-1262-484a-8efd-6e1eeada944d__process.patch


Thanks,

Andy Pang

Re: Review Request 58533: Added a benchmark test for hierarchical sorter.

[Mesos Reviewbot]

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



Bad review!

Reviews applied: [58533, 57516, 57254, 58112, 58110, 57564, 57528, 57527, 
57788, 57167, 57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot


On April 20, 2017, 3:54 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58533/
> ---
> 
> (Updated April 20, 2017, 3:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7395
> https://issues.apache.org/jira/browse/MESOS-7395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is very similar to Sorter_BENCHMARK_Test.FullSort, except
> that hierarchical role is built instead of flat one.
> 
> 
> Diffs
> -
> 
>   src/tests/sorter_tests.cpp 43bd85798aef0c89751b725ebf35308a5e9e997a 
> 
> 
> Diff: https://reviews.apache.org/r/58533/diff/2/
> 
> 
> Testing
> ---
> 
> `./bin/mesos-tests.sh --benchmark 
> --gtest_filter="AgentAndClientCount/HSorter_BENCHMARK_Test.FullSort*"`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>