Re: Review Request 40244: Clarified comments in Master::Http::_operation.

[Michael Park]

> On Nov. 18, 2015, 8:05 a.m., Qian Zhang wrote:
> > src/master/http.cpp, line 2097
> > 
> >
> > This comment may not be related to this patch. I am just curious, for 
> > ```required```, we have called ```flatten()``` to remove role from it when 
> > passing it into ```Master::Http::_operation()```. But here for 
> > ```offer->resources()```, we do not call ```flatten()``` to remove its 
> > role, so is it subtractable between ```required``` and 
> > ```offer->resources()```?
> 
> Michael Park wrote:
> We `flatten` the resources for `/reserve` because the `required` 
> represents the resources that we need in order to perform the reservation.
> Suppose we want to end up with 2 reserved cpus. We `flatten` it, and 
> search for 2 unreserved cpus in the received offers.
> If we were to `flatten` the `offer.resources()` also, then an offer with 
> 1 reserved cpu and 1 unreserved cpu would incorrectly satisfy our condition.
> 
> In terms of subtractibility, that is determined on a per-resource basis, 
> and non-subtractable resources are ignored.
> For example, `[(2 unreserved cpus)] - [(1 reserved cpu), (1 unreserved 
> cpu)]` == `[(1 unreserved cpu)]`.
> 
> Qian Zhang wrote:
> Suppose ```offer.resources()``` is ```[(1 reserved cpu), (1 unreserved 
> cpu)]```, and ```required``` is ```[(2 unreserved cpus)]```, so the result of 
> ```required == required - offer.resources()``` is ```false```, that mean we 
> will rescind this offer, right? But it may not make sense to me, because I 
> think the **reserved** resource should not be rescinded.

Yes, it's clear that this approach has limitations. It was just the best known 
approach given the current state of things since we cannot currently rescind 
offers partially. While rescinding the __reserved__ resource is undesirable, it 
still meets the requirements as the resource will still be offered to the role.


- Michael


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


On Nov. 18, 2015, 11:27 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40244/
> ---
> 
> (Updated Nov. 18, 2015, 11:27 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Bugs: MESOS-2455
> https://issues.apache.org/jira/browse/MESOS-2455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified comments in Master::Http::_operation.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
> 
> Diff: https://reviews.apache.org/r/40244/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40371: Changed mesos-execute to add containerizer option.

[Jojy Varghese]

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

(Updated Nov. 20, 2015, 7:45 a.m.)


Review request for mesos and Timothy Chen.


Repository: mesos


Description
---

Since docker_image option could be used for mesos and docker containerizer,
introduced a  new option 'containerizer' to disambiguate the two containerizers.

New usage:
mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
--name=test_docker --docker_image=ubuntu --containerizer=docker

mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
--name=test_mesos --docker_image=ubuntu --containerizer=mesos


Diffs
-

  src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc 

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


Testing
---

tested the two containerizer locally with simple commands.


Thanks,

Jojy Varghese

Review Request 40531: Added the public Mesos events calendar to the Community page.

[Michael Park]

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

Review request for mesos and Dave Lester.


Repository: mesos


Description
---

See summary.


Diffs
-

  site/source/community.html.md 81b8d3d02739fb624b599e002df6bc5274381453 

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


Testing
---

Used `support/site-docker` to render the calendar locally.


File Attachments


Screen Shot 2015-11-19 at 11.34.17 PM.png
  
https://reviews.apache.org/media/uploaded/files/2015/11/20/b39ee774-96f7-4afb-b6db-adcfa8bab9aa__Screen_Shot_2015-11-19_at_11.34.17_PM.png


Thanks,

Michael Park

Review Request 40529: WIP: Added helper function to get stateless resources.

[Guangya Liu]

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

Review request for mesos and Klaus Ma.


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


Repository: mesos


Description
---

Added helper function to get stateless resources.


Diffs
-

  include/mesos/resources.hpp ce12a26e9cc9057d6612cf2380c07be959e2990b 
  include/mesos/v1/resources.hpp 92db123507ba3442d475c5d3bb7e1a375cf5c1b2 
  src/common/resources.cpp f1da3237724b3766a5df1cb0a4c0159fb9098e01 
  src/tests/resources_tests.cpp 0d084fd97ec108d5ec2d050eddc2e80ea81ffac0 
  src/v1/resources.cpp 6f50101538cf30ebb5a8022558108f103d62a44c 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [39415, 39416, 40497, 40506, 39417]

Failed command: ./support/apply-review.sh -n -r 39417

Error:
 2015-11-20 07:23:28 URL:https://reviews.apache.org/r/39417/diff/raw/ 
[9792/9792] -> "39417.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/isolators/network/port_mapping.cpp:1385
error: src/slave/containerizer/mesos/isolators/network/port_mapping.cpp: patch 
does not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 19, 2015, 10:59 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> ---
> 
> (Updated Nov. 19, 2015, 10:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When --egress_unique_flow_per_container is enabled, we need to install a flow 
> classifier (fq_codel) qdisc on egress side. This flag specifies where to 
> install it in the hierarchy. By default, we install it at root. But, for 
> example, if you have already installed HTB qdisc at root, you may want this 
> to be installed in other place than root, specify an HTB class ID as its 
> parent here.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> cc0152ce44e9fa34210efda8a1c4a6374167eab1 
>   src/slave/flags.hpp 6ae7c94d2e05d81c8b970e7dcaa82d8aa4de7936 
>   src/slave/flags.cpp 26f554e1a73f1f7f3d7baec7bfc1a7f456c5677c 
> 
> Diff: https://reviews.apache.org/r/39417/diff/
> 
> 
> Testing
> ---
> 
> Manual tests, with and without a pre-installed HTB qdisc and classes.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Yong Qiao Wang]

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

(Updated Nov. 20, 2015, 6:42 a.m.)


Review request for mesos, Adam B, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

Move RoleInfo message out of allocator.proto


Diffs (updated)
-

  include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
  include/mesos/master/allocator.proto 224da71e9f34d2ea11a6e6e235d0f8196abaeb90 
  include/mesos/role/role.hpp PRE-CREATION 
  include/mesos/role/role.proto PRE-CREATION 
  src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
  src/master/allocator/mesos/allocator.hpp 
d2d32af227d66c4030becd4cd64b907a70d25f49 
  src/master/allocator/mesos/hierarchical.hpp 
64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
  src/master/allocator/mesos/hierarchical.cpp 
f2e3b639f210eb06c70584ee7294609d9fd978ad 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
  src/tests/hierarchical_allocator_tests.cpp 
740cfa801ee90417c038308192d1f4f2416f8315 

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


Testing
---

1. Make Check successfully;

2. $ curl http://9.110.48.168:5050/roles
{"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}


Thanks,

Yong Qiao Wang

Re: Review Request 40524: WIP: Enabled std::string namespace for resources.hpp

[Klaus Ma]

> On Nov. 20, 2015, 2:03 p.m., Klaus Ma wrote:
> > include/mesos/resources.hpp, line 41
> > 
> >
> > Generally, we did not use `using` in header in C++; it may conflict 
> > with the `using` in cpp files.
> > 
> > I grep header files in the code, some header file did not follow this 
> > practice. Maybe we should correct them.

Just copy the google code style about using:

>>You may use a using-declaration anywhere in a .cc file (including in the 
>>global namespace), and in functions, methods, classes, or within internal 
>>namespaces in .h files.

>>Do not use using-declarations in .h files except in explicitly marked 
>>internal-only namespaces, because anything imported into a namespace in a .h 
>>file becomes part of the public API exported by that file.
```
// OK in .cc files.
// Must be in a function, method, internal namespace, or
// class in .h files.
using ::foo::bar;
```


- Klaus


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


On Nov. 20, 2015, 1:54 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40524/
> ---
> 
> (Updated Nov. 20, 2015, 1:54 p.m.)
> 
> 
> Review request for mesos and Klaus Ma.
> 
> 
> Bugs: MESOS-3955
> https://issues.apache.org/jira/browse/MESOS-3955
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled std::string namespace for resources.hpp
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp ce12a26e9cc9057d6612cf2380c07be959e2990b 
>   include/mesos/v1/resources.hpp 92db123507ba3442d475c5d3bb7e1a375cf5c1b2 
> 
> Diff: https://reviews.apache.org/r/40524/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39223: Added Quota Request Validation Tests.

[Joerg Schad]

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



src/tests/master_quota_tests.cpp (line 332)


Move before StartSlave here and below.


- Joerg Schad


On Nov. 19, 2015, 9:39 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39223/
> ---
> 
> (Updated Nov. 19, 2015, 9:39 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3199
> https://issues.apache.org/jira/browse/MESOS-3199
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see Summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39223/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40524: WIP: Enabled std::string namespace for resources.hpp

[Klaus Ma]

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



include/mesos/resources.hpp (line 41)


Generally, we did not use `using` in header in C++; it may conflict with 
the `using` in cpp files.

I grep header files in the code, some header file did not follow this 
practice. Maybe we should correct them.


- Klaus Ma


On Nov. 20, 2015, 1:54 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40524/
> ---
> 
> (Updated Nov. 20, 2015, 1:54 p.m.)
> 
> 
> Review request for mesos and Klaus Ma.
> 
> 
> Bugs: MESOS-3955
> https://issues.apache.org/jira/browse/MESOS-3955
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled std::string namespace for resources.hpp
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp ce12a26e9cc9057d6612cf2380c07be959e2990b 
>   include/mesos/v1/resources.hpp 92db123507ba3442d475c5d3bb7e1a375cf5c1b2 
> 
> Diff: https://reviews.apache.org/r/40524/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Joerg Schad]

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



src/tests/master_quota_tests.cpp (line 482)


Move Expect call before StartSlave()


- Joerg Schad


On Nov. 19, 2015, 4:05 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40396: Quota: Added a test for offer rescinding.

[Joerg Schad]

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



src/tests/master_quota_tests.cpp (line 454)


This should be before StartSlave() as otherwise the test might be flaky (in 
cases where the slave has been added before the Expect_Call)



src/tests/master_quota_tests.cpp (lines 465 - 466)


same here



src/tests/master_quota_tests.cpp (line 478)


same here


- Joerg Schad


On Nov. 17, 2015, 8:18 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40396/
> ---
> 
> (Updated Nov. 17, 2015, 8:18 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3912
> https://issues.apache.org/jira/browse/MESOS-3912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40396/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38956: Quota: Added allocator-agnostic tests.

[Joerg Schad]

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



src/tests/master_quota_tests.cpp (line 222)


Shouldn't the expect call happen before StartSlave()? Otherwise the slave 
might have started before and we will never satisfy the call...



src/tests/master_quota_tests.cpp (line 270)


move before StartSlave()



src/tests/master_quota_tests.cpp (line 282)


Move before StartSlave()



src/tests/master_quota_tests.cpp (line 332)


Move before StartSlave()



src/tests/master_quota_tests.cpp (line 383)


Shouldn't we have the expect call before StartSlave()? Otherwise this is a 
potential race condition which might lead to the ExpectCall never fullfilled 
(as the slave already started before we started expecting it).



src/tests/master_quota_tests.cpp (line 395)


Same here.


- Joerg Schad


On Nov. 17, 2015, 8:16 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38956/
> ---
> 
> (Updated Nov. 17, 2015, 8:16 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3720
> https://issues.apache.org/jira/browse/MESOS-3720
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38956/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40524: WIP: Enabled std::string namespace for resources.hpp

[Guangya Liu]

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

(Updated 十一月 20, 2015, 5:54 a.m.)


Review request for mesos and Klaus Ma.


Summary (updated)
-

WIP: Enabled std::string namespace for resources.hpp


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


Repository: mesos


Description
---

Enabled std::string namespace for resources.hpp


Diffs (updated)
-

  include/mesos/resources.hpp ce12a26e9cc9057d6612cf2380c07be959e2990b 
  include/mesos/v1/resources.hpp 92db123507ba3442d475c5d3bb7e1a375cf5c1b2 

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


Testing
---


Thanks,

Guangya Liu

Review Request 40524: Enabled std::string namespace for resources.hpp

[Guangya Liu]

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

Review request for mesos and Klaus Ma.


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


Repository: mesos


Description
---

Enabled std::string namespace for resources.hpp


Diffs
-

  include/mesos/resources.hpp ce12a26e9cc9057d6612cf2380c07be959e2990b 
  include/mesos/v1/resources.hpp 92db123507ba3442d475c5d3bb7e1a375cf5c1b2 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Joerg Schad]

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

(Updated Nov. 20, 2015, 5:44 a.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Changes
---

Restored blank line.


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


Repository: mesos


Description
---

Added force flag to override quota capacityHeuristic check.


Diffs (updated)
-

  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Joerg Schad

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Joerg Schad]

> On Nov. 19, 2015, 11:10 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 460
> > 
> >
> > Could you please restore this blank line?

Sure, but FYI this style is consistent with the the rest of the file where we 
have one single line between TODO and first test (see line 155/156)


- Joerg


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


On Nov. 19, 2015, 7:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40392/
> ---
> 
> (Updated Nov. 19, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3911
> https://issues.apache.org/jira/browse/MESOS-3911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added force flag to override quota capacityHeuristic check.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40392/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40501: Cleanup a leaked reference to a test process living in the stack.

[Bernd Mathiske]

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

Ship it!


Great catach!

- Bernd Mathiske


On Nov. 19, 2015, 1:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> ---
> 
> (Updated Nov. 19, 2015, 1:51 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Two of the fetcher tests will spawn a process which is stored in the stack 
> (i.e. local variable in the test).  `spawn` will store a pointer to the 
> process in libprocess's `ProcessManager`.  When the test finishes, the 
> process goes out of scope and is therefore lost.  However, the process is 
> **not** terminated.
> 
> Failing to terminate this process will lead to an infinite loop in 
> `~ProcessManager`, which is called in `process::finalize`.  In 
> `ProcessManager` 's destructor, we will loop and try to kill all processes.  
> The process spawned in the test will be running.  However, since the pointer 
> lives in the stack, the `ProcessManager` will be unable to find the process 
> and will thereby be stuck trying to kill a process it cannot find.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8 
> 
> Diff: https://reviews.apache.org/r/40501/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional testing:
> 
> Insert a `process::finalize` in `src/test/main.cpp`.  i.e.
> ```
>   // Replace `return RUN_ALL_TESTS();` with this:
>   int ret = RUN_ALL_TESTS();
>   process::finalize();
>   return ret;
> ```
> 
> Then `make check 
> GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40501: Cleanup a leaked reference to a test process living in the stack.

[Bernd Mathiske]

> On Nov. 19, 2015, 6:16 p.m., Neil Conway wrote:
> > Good find. I wonder:
> > 
> > (a) is there some general advice we should give to people implementing 
> > Processes (e.g., "always provide a destructor that does terminate/wait" -- 
> > that is probably too broad though). Would be nice to add this to the 
> > libprocess README.
> > (b) does this problem occur anywhere else? and/or is there a way to detect 
> > it?

[~neilc] a) I agree that we should provide documentation in this regard. This 
kind of pattern is too easily overlooked, also by reviewers, as exemplified 
when the bug got introduced: https://reviews.apache.org/r/27483/
b) For test code (such as is the case here), we could put something into the 
inherited fixture that scans for orphaned processes at TearDown.


- Bernd


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


On Nov. 19, 2015, 1:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> ---
> 
> (Updated Nov. 19, 2015, 1:51 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Two of the fetcher tests will spawn a process which is stored in the stack 
> (i.e. local variable in the test).  `spawn` will store a pointer to the 
> process in libprocess's `ProcessManager`.  When the test finishes, the 
> process goes out of scope and is therefore lost.  However, the process is 
> **not** terminated.
> 
> Failing to terminate this process will lead to an infinite loop in 
> `~ProcessManager`, which is called in `process::finalize`.  In 
> `ProcessManager` 's destructor, we will loop and try to kill all processes.  
> The process spawned in the test will be running.  However, since the pointer 
> lives in the stack, the `ProcessManager` will be unable to find the process 
> and will thereby be stuck trying to kill a process it cannot find.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8 
> 
> Diff: https://reviews.apache.org/r/40501/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional testing:
> 
> Insert a `process::finalize` in `src/test/main.cpp`.  i.e.
> ```
>   // Replace `return RUN_ALL_TESTS();` with this:
>   int ret = RUN_ALL_TESTS();
>   process::finalize();
>   return ret;
> ```
> 
> Then `make check 
> GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40339: WIP: Added a flag to master to enable optimistic offers.

[Guangya Liu]

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

(Updated 十一月 20, 2015, 3:37 a.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Klaus Ma.


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


Repository: mesos


Description
---

Added a flag to master to enable optimistic offers.


Diffs (updated)
-

  include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
  src/master/allocator/mesos/allocator.hpp 
d2d32af227d66c4030becd4cd64b907a70d25f49 
  src/master/allocator/mesos/hierarchical.hpp 
64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
  src/master/allocator/mesos/hierarchical.cpp 
f2e3b639f210eb06c70584ee7294609d9fd978ad 
  src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
  src/master/flags.cpp 806e2da6ad37a6acf76818d4c6b3c462175fd09d 
  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
  src/tests/hierarchical_allocator_tests.cpp 
740cfa801ee90417c038308192d1f4f2416f8315 
  src/tests/master_allocator_tests.cpp 646cacd3e16b9e0b72c0b259eecf2760cfb530db 
  src/tests/reservation_endpoints_tests.cpp 
1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
  src/tests/resource_offers_tests.cpp bf2fe3ac7b982e31d289a703ae637d8e2b3a2d8a 
  src/tests/slave_recovery_tests.cpp 2cc7132deb9b8c324aa9dbab0b81643d07377a89 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 40339: WIP: Added a flag to master to enable optimistic offers.

[Guangya Liu]

> On 十一月 19, 2015, 5:59 p.m., Joseph Wu wrote:
> > Overall notes:
> > * Looks like everything is in place (to my knowledge) for this change :)
> > * (Mentioned in the last working group sync) We **may** want to rename the 
> > feature to something else, like "oversubscription for reservations".

Agree, I will update the name to "oversubscription for reservations"


- Guangya


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


On 十一月 20, 2015, 3:25 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40339/
> ---
> 
> (Updated 十一月 20, 2015, 3:25 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Klaus 
> Ma.
> 
> 
> Bugs: MESOS-3887
> https://issues.apache.org/jira/browse/MESOS-3887
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag to master to enable optimistic offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
>   src/master/allocator/mesos/allocator.hpp 
> d2d32af227d66c4030becd4cd64b907a70d25f49 
>   src/master/allocator/mesos/hierarchical.hpp 
> 64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
>   src/master/allocator/mesos/hierarchical.cpp 
> f2e3b639f210eb06c70584ee7294609d9fd978ad 
>   src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
>   src/master/flags.cpp 806e2da6ad37a6acf76818d4c6b3c462175fd09d 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
>   src/tests/hierarchical_allocator_tests.cpp 
> 740cfa801ee90417c038308192d1f4f2416f8315 
>   src/tests/master_allocator_tests.cpp 
> 646cacd3e16b9e0b72c0b259eecf2760cfb530db 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
>   src/tests/resource_offers_tests.cpp 
> bf2fe3ac7b982e31d289a703ae637d8e2b3a2d8a 
>   src/tests/slave_recovery_tests.cpp 2cc7132deb9b8c324aa9dbab0b81643d07377a89 
> 
> Diff: https://reviews.apache.org/r/40339/diff/
> 
> 
> Testing
> ---
> 
> Ubuntu 14.04
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40339: WIP: Added a flag to master to enable optimistic offers.

[Guangya Liu]

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

(Updated 十一月 20, 2015, 3:25 a.m.)


Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Klaus Ma.


Summary (updated)
-

WIP: Added a flag to master to enable optimistic offers.


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


Repository: mesos


Description
---

Added a flag to master to enable optimistic offers.


Diffs (updated)
-

  include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
  src/master/allocator/mesos/allocator.hpp 
d2d32af227d66c4030becd4cd64b907a70d25f49 
  src/master/allocator/mesos/hierarchical.hpp 
64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
  src/master/allocator/mesos/hierarchical.cpp 
f2e3b639f210eb06c70584ee7294609d9fd978ad 
  src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
  src/master/flags.cpp 806e2da6ad37a6acf76818d4c6b3c462175fd09d 
  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
  src/tests/hierarchical_allocator_tests.cpp 
740cfa801ee90417c038308192d1f4f2416f8315 
  src/tests/master_allocator_tests.cpp 646cacd3e16b9e0b72c0b259eecf2760cfb530db 
  src/tests/reservation_endpoints_tests.cpp 
1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
  src/tests/resource_offers_tests.cpp bf2fe3ac7b982e31d289a703ae637d8e2b3a2d8a 
  src/tests/slave_recovery_tests.cpp 2cc7132deb9b8c324aa9dbab0b81643d07377a89 

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


Testing
---

Ubuntu 14.04
make
make check


Thanks,

Guangya Liu

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Qian Zhang]

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



src/Makefile.am (line 955)


Indent


- Qian Zhang


On Nov. 20, 2015, 10:01 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40431/
> ---
> 
> (Updated Nov. 20, 2015, 10:01 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3944
> https://issues.apache.org/jira/browse/MESOS-3944
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move RoleInfo message out of allocator.proto
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
>   include/mesos/master/allocator.proto 
> 224da71e9f34d2ea11a6e6e235d0f8196abaeb90 
>   include/mesos/role/role.hpp PRE-CREATION 
>   include/mesos/role/role.proto PRE-CREATION 
>   src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
>   src/master/allocator/mesos/allocator.hpp 
> d2d32af227d66c4030becd4cd64b907a70d25f49 
>   src/master/allocator/mesos/hierarchical.hpp 
> 64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
>   src/master/allocator/mesos/hierarchical.cpp 
> f2e3b639f210eb06c70584ee7294609d9fd978ad 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
>   src/tests/hierarchical_allocator_tests.cpp 
> 740cfa801ee90417c038308192d1f4f2416f8315 
> 
> Diff: https://reviews.apache.org/r/40431/diff/
> 
> 
> Testing
> ---
> 
> 1. Make Check successfully;
> 
> 2. $ curl http://9.110.48.168:5050/roles
> {"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.

[Qian Zhang]

> On Nov. 18, 2015, 3:51 p.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Why do we want to rescind the offeres that do not contribute to 
> > satisfying quota request?
> 
> Alexander Rukletsov wrote:
> Because we may rescind more than necessary to satisfy quota request 
> (remember minimal agent count). If we have a check in place, this will 
> effectively prevent us from doing so. Does it make sense to you?

Suppose the quota request is to request 20GB disk for a role, and there is an 
offer which only include 2 CPU & 2GB memory and has no disk resources at all, 
so we will rescind this offer too? This seems a little unfair to me.
And can you please clarify a little more about why we want to rescind offers 
from at least `numF` agents? The reason is that we want to ensure each 
framework in that role will have a chance to get an offer in next allocation 
cycle?


- Qian


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


On Nov. 20, 2015, 1:15 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40351/
> ---
> 
> (Updated Nov. 20, 2015, 1:15 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3912
> https://issues.apache.org/jira/browse/MESOS-3912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40351/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40459: Added 2 slave flags --advertise_ip and --advertise_port.

[Guangya Liu]

> On 十一月 19, 2015, 2:57 a.m., Guangya Liu wrote:
> > docs/configuration.md, lines 37-54
> > 
> >
> > I think that the master/main.cpp should also be updated for those 
> > flages.
> 
> Anindya Sinha wrote:
> It is already there added as a part of MESOS-809. Refer 
> https://github.com/apache/mesos/blob/master/src/master/main.cpp#L131 and 
> https://github.com/apache/mesos/blob/master/src/master/main.cpp#L138.
> 
> Guangya Liu wrote:
> I mean this part should also be updated to use the latest description as 
> here you have updated "mesos master" to "mesos master/slave"
> 
> Anindya Sinha wrote:
> Sorry, I am not sure I understand the concern. Are you suggesting we 
> should have the exact same text when we do "mesos-master --help" or 
> "mesos-slave --help" or look in the documentation.
> 
> In docs/configuration.md, I moved description of advertise_ip and 
> advertise_port to the section of "Master and Slave Options" from the original 
> "Master options" to denote the flags correspond to both master as well as 
> slave (and the text reflects that). And it uses "mesos master/slave" since 
> this is a common command line arg.
> In src/slave/main.cpp: It has the same language as in 
> docs/configuration.md except that I mention "slave" instead of "master/slave".
> In src/master/main.cpp: It has the same language as in 
> docs/configuration.md except that I mention "master" instead of 
> "master/slave".
> 
> So when we do "mesos-master --help", we see mesos master (not mesos 
> master/slave).
> And when we do "mesos-slave --help", we see mesos slave (not mesos 
> master/slave).
> But in documentation, we see mesos master/slave (since these 2 flags are 
> common to both mesos master as well as mesos slave).

Got it, thanks!


- Guangya


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


On 十一月 19, 2015, 12:58 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40459/
> ---
> 
> (Updated 十一月 19, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Timothy Chen.
> 
> 
> Bugs: MESOS-3809
> https://issues.apache.org/jira/browse/MESOS-3809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If set, these IP/Port shall be advertised by libprocess (although bind is not 
> done on this IP/Port). If not set, libprocess advertises the IP/Port on which 
> bind was done.
> 
> Command line arguments added:
> advertise_ip: IP address advertised to reach mesos slave.
> advertise_port: Port advertised to reach mesos slave (used with advertise_ip).
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
>   src/slave/main.cpp 746eec684a0c9119f85506022676d276f808b6ea 
> 
> Diff: https://reviews.apache.org/r/40459/diff/
> 
> 
> Testing
> ---
> 
> make check run successfully.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 40501: Cleanup a leaked reference to a test process living in the stack.

[Neil Conway]

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

Ship it!


Good find. I wonder:

(a) is there some general advice we should give to people implementing 
Processes (e.g., "always provide a destructor that does terminate/wait" -- that 
is probably too broad though). Would be nice to add this to the libprocess 
README.
(b) does this problem occur anywhere else? and/or is there a way to detect it?

- Neil Conway


On Nov. 19, 2015, 9:51 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40501/
> ---
> 
> (Updated Nov. 19, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3753
> https://issues.apache.org/jira/browse/MESOS-3753
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Two of the fetcher tests will spawn a process which is stored in the stack 
> (i.e. local variable in the test).  `spawn` will store a pointer to the 
> process in libprocess's `ProcessManager`.  When the test finishes, the 
> process goes out of scope and is therefore lost.  However, the process is 
> **not** terminated.
> 
> Failing to terminate this process will lead to an infinite loop in 
> `~ProcessManager`, which is called in `process::finalize`.  In 
> `ProcessManager` 's destructor, we will loop and try to kill all processes.  
> The process spawned in the test will be running.  However, since the pointer 
> lives in the stack, the `ProcessManager` will be unable to find the process 
> and will thereby be stuck trying to kill a process it cannot find.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8 
> 
> Diff: https://reviews.apache.org/r/40501/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Additional testing:
> 
> Insert a `process::finalize` in `src/test/main.cpp`.  i.e.
> ```
>   // Replace `return RUN_ALL_TESTS();` with this:
>   int ret = RUN_ALL_TESTS();
>   process::finalize();
>   return ret;
> ```
> 
> Then `make check 
> GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
> The test program should not stall or segfault or abort in some weird way.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40244: Clarified comments in Master::Http::_operation.

[Qian Zhang]

> On Nov. 18, 2015, 4:05 p.m., Qian Zhang wrote:
> > src/master/http.cpp, line 2097
> > 
> >
> > This comment may not be related to this patch. I am just curious, for 
> > ```required```, we have called ```flatten()``` to remove role from it when 
> > passing it into ```Master::Http::_operation()```. But here for 
> > ```offer->resources()```, we do not call ```flatten()``` to remove its 
> > role, so is it subtractable between ```required``` and 
> > ```offer->resources()```?
> 
> Michael Park wrote:
> We `flatten` the resources for `/reserve` because the `required` 
> represents the resources that we need in order to perform the reservation.
> Suppose we want to end up with 2 reserved cpus. We `flatten` it, and 
> search for 2 unreserved cpus in the received offers.
> If we were to `flatten` the `offer.resources()` also, then an offer with 
> 1 reserved cpu and 1 unreserved cpu would incorrectly satisfy our condition.
> 
> In terms of subtractibility, that is determined on a per-resource basis, 
> and non-subtractable resources are ignored.
> For example, `[(2 unreserved cpus)] - [(1 reserved cpu), (1 unreserved 
> cpu)]` == `[(1 unreserved cpu)]`.

Suppose ```offer.resources()``` is ```[(1 reserved cpu), (1 unreserved 
cpu)]```, and ```required``` is ```[(2 unreserved cpus)]```, so the result of 
```required == required - offer.resources()``` is ```false```, that mean we 
will rescind this offer, right? But it may not make sense to me, because I 
think the **reserved** resource should not be rescinded.


- Qian


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


On Nov. 18, 2015, 7:27 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40244/
> ---
> 
> (Updated Nov. 18, 2015, 7:27 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Bugs: MESOS-2455
> https://issues.apache.org/jira/browse/MESOS-2455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified comments in Master::Http::_operation.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
> 
> Diff: https://reviews.apache.org/r/40244/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40431: Move RoleInfo message out of allocator.proto

[Yong Qiao Wang]

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

(Updated Nov. 20, 2015, 2:01 a.m.)


Review request for mesos, Adam B, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

Move RoleInfo message out of allocator.proto


Diffs (updated)
-

  include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
  include/mesos/master/allocator.proto 224da71e9f34d2ea11a6e6e235d0f8196abaeb90 
  include/mesos/role/role.hpp PRE-CREATION 
  include/mesos/role/role.proto PRE-CREATION 
  src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
  src/master/allocator/mesos/allocator.hpp 
d2d32af227d66c4030becd4cd64b907a70d25f49 
  src/master/allocator/mesos/hierarchical.hpp 
64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
  src/master/allocator/mesos/hierarchical.cpp 
f2e3b639f210eb06c70584ee7294609d9fd978ad 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
  src/tests/hierarchical_allocator_tests.cpp 
740cfa801ee90417c038308192d1f4f2416f8315 

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


Testing
---

1. Make Check successfully;

2. $ curl http://9.110.48.168:5050/roles
{"roles":[{"frameworks":[],"name":"*","resources":{"cpus":0,"disk":0,"mem":0},"weight":1.0}]}


Thanks,

Yong Qiao Wang

Re: Review Request 40469: Update Allocator interface to support dynamic roles

[Yong Qiao Wang]

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

(Updated Nov. 20, 2015, 2:02 a.m.)


Review request for mesos, Adam B, Guangya Liu, and Qian Zhang.


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


Repository: mesos


Description
---

Update Allocator interface to support dynamic roles


Diffs (updated)
-

  include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
  src/master/allocator/mesos/allocator.hpp 
d2d32af227d66c4030becd4cd64b907a70d25f49 
  src/master/allocator/mesos/hierarchical.hpp 
64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
  src/master/allocator/mesos/hierarchical.cpp 
f2e3b639f210eb06c70584ee7294609d9fd978ad 
  src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 

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


Testing
---

Make check successfully.


Thanks,

Yong Qiao Wang

Re: Review Request 39782: Add a comment for os::libraries::setPaths.

[James Peach]

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

(Updated Nov. 20, 2015, 1:27 a.m.)


Review request for mesos, Kapil Arya and Niklas Nielsen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add a comment for os::libraries::setPaths.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
5c1df81193b4b888d2ed5c7dbfa0b5e2fae48467 

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


Testing
---

No code changes.


Thanks,

James Peach

Re: Review Request 39781: Update ModuleTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Nov. 20, 2015, 1:27 a.m.)


Review request for mesos, Kapil Arya and Niklas Nielsen.


Changes
---

Rebase and fix formatting warnings.


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


Repository: mesos


Description
---

Update ModuleTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/module_tests.cpp 28c71b0c1960bad4933f86d35fe8a0248fef326e 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39780: Update OversubscriptionTest to not assume dynamic dlopen search.

[James Peach]

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

(Updated Nov. 20, 2015, 1:27 a.m.)


Review request for mesos, Kapil Arya and Niklas Nielsen.


Changes
---

Rebase


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


Repository: mesos


Description
---

Update OversubscriptionTest to not assume dynamic dlopen search.


Diffs (updated)
-

  src/tests/oversubscription_tests.cpp 0d0bf7e0b9a4028ed116e00b56d59b670510c5ce 

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


Testing
---

make check


Thanks,

James Peach

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Jie Yu]

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



src/master/master.cpp (lines 3016 - 3028)


Let's move the validation to aurhtorizeReserveResources.

You also need to add a NOTE in `_accept` saying that no need to validate 
the operation again since it's already validated during authorization.



src/master/master.cpp (lines 3053 - 3054)


I don't think the comments here is necessary. We need to avoid 
over-commenting. If the code itself is quite clear about what it's about to do, 
no need for the extra comments here.



src/master/master.cpp (line 3160)


No need for this comment.



src/master/master.cpp (line 3195)


I don't think this comment add more value. The code itself is quite clear 
what it is about to do.



src/master/master.cpp (line 3202)


Ditto.



src/master/master.cpp (line 3206)


Remove this comment.



src/master/master.cpp (line 3237)


Ditto.



src/master/master.cpp (line 3310)


Ditto.



src/tests/reservation_tests.cpp (lines 1657 - 1668)


This might be flaky because master might send out an offer before it 
processes the terminal status update.


- Jie Yu


On Nov. 20, 2015, 12:20 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 20, 2015, 12:20 a.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

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

(Updated Nov. 20, 2015, 12:20 a.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Removed unwanted changes.


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


Repository: mesos


Description
---

Added framework authorization for dynamic reservation.
Note: this review is continued from https://reviews.apache.org/r/37127/


Diffs (updated)
-

  src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
  src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 

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


Testing
---

This is the fifth in a chain of 5 patches. New reservation tests were added to 
`reservation_tests.cpp` to validate the authentication of framework reserve and 
unreserve operations using ACLs. `make check` was run to test after all patches 
were applied.


Thanks,

Greg Mann

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

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



src/master/master.cpp (lines 3278 - 3288)


Whoops, these changes from later on in the chain crept in here. I'll remove 
them.


- Greg Mann


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 19, 2015, 11:41 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Nov. 19, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39987: [3/5] Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.

[Jie Yu]

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

Ship it!



src/master/master.cpp (line 2792)


You don't need the 'stringify' here because '<<' operator overload will 
call stringify for you.


- Jie Yu


On Nov. 16, 2015, 11:48 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39987/
> ---
> 
> (Updated Nov. 16, 2015, 11:48 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added 'Master::authorize(Un)reserveResources()' for Reserve/Unreserve.
> Note: this review is continued from https://reviews.apache.org/r/37125/
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
> 
> Diff: https://reviews.apache.org/r/39987/diff/
> 
> 
> Testing
> ---
> 
> This is the third in a chain of 5 patches. `make check` was used to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40511: Disabled docker bridge executor test.

[Jie Yu]

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



src/tests/containerizer/docker_containerizer_tests.cpp (lines 282 - 284)


Please wrap the comments consistently here. This wrapping here is very 
wiered.


- Jie Yu


On Nov. 20, 2015, midnight, Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40511/
> ---
> 
> (Updated Nov. 20, 2015, midnight)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Marco Massenzio.
> 
> 
> Bugs: MESOS-3123
> https://issues.apache.org/jira/browse/MESOS-3123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Disabled docker bridge executor test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d2a1ccc483cac51aee7a1fa57dae36637d34973a 
> 
> Diff: https://reviews.apache.org/r/40511/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 40511: Disabled docker bridge executor test.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 20, 2015, midnight, Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40511/
> ---
> 
> (Updated Nov. 20, 2015, midnight)
> 
> 
> Review request for mesos, Bernd Mathiske, Jie Yu, and Marco Massenzio.
> 
> 
> Bugs: MESOS-3123
> https://issues.apache.org/jira/browse/MESOS-3123
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Disabled docker bridge executor test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> d2a1ccc483cac51aee7a1fa57dae36637d34973a 
> 
> Diff: https://reviews.apache.org/r/40511/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Review Request 40511: Disabled docker bridge executor test.

[Timothy Chen]

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

Review request for mesos, Bernd Mathiske, Jie Yu, and Marco Massenzio.


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


Repository: mesos


Description
---

Disabled docker bridge executor test.


Diffs
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
d2a1ccc483cac51aee7a1fa57dae36637d34973a 

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


Testing
---


Thanks,

Timothy Chen

Re: Review Request 39416: Document --egress_unique_flow_per_container in docs/configuration.md

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 19, 2015, 8:09 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39416/
> ---
> 
> (Updated Nov. 19, 2015, 8:09 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Flag --egress_unique_flow_per_container is documented in help message, but 
> not in docs/configuration.md. We should document it there too, with more 
> details.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
> 
> Diff: https://reviews.apache.org/r/39416/diff/
> 
> 
> Testing
> ---
> 
> None.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

> On Nov. 13, 2015, 11:38 p.m., Jie Yu wrote:
> > src/master/http.cpp, line 853
> > 
> >
> > This looks problematic to me. 'this' will become invalid once this 
> > function returns. That means when `_reserve` is actually called after the 
> > authorization is done, it might be a dangling pointer.
> > 
> > Instead, you can try to copy the http object (something like below):
> > 
> > ```
> > lambda::function(bool)> _reserve =
> >   lambda::bind(
> >   &Master::Http::_operation,
> >   *this,
> >   slaveId,
> >   resources.flatten(),
> >   operation,
> >   lambda::_1);
> > 
> > return master->authorizeReserveResources(operation.reserve(), principal)
> >   .then(defer(master->self(), _reserve));
> > ```
> 
> Greg Mann wrote:
> Good catch, thanks!
> 
> Jie Yu wrote:
> Mpark reached out to me about this issue. Seems that this extra copying 
> is not necessary (my bad). The 'http' object will be bounded in the lambda 
> function and stored in Master's libprocess handlers map.

I checked out the IRC logs; that makes sense! I reverted back to passing `this`.


- Greg


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


On Nov. 19, 2015, 11:41 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Nov. 19, 2015, 11:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Greg Mann]

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

(Updated Nov. 19, 2015, 11:41 p.m.)


Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added authorization for dynamic reservation master endpoints.
Note: this review is continued from https://reviews.apache.org/r/37126/


Diffs (updated)
-

  src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/tests/reservation_endpoints_tests.cpp 
1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 

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


Testing
---

This is the fourth in a chain of 5 patches. Added new reservation endpoints 
tests to validate authorization of reserve and unreserve operations using ACLs. 
`make check` was run to test after all patches were applied.


Thanks,

Greg Mann

Re: Review Request 40507: Cleanup leaked containerizer and potentially orphaned process in SlaveTest.LaunchTaskInfoWithContainerInfo.

[Joseph Wu]

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

(Updated Nov. 19, 2015, 3:38 p.m.)


Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


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


Repository: mesos


Description
---

Normally, the containerizer would be cleaned and deallocated by one of the 
helper methods in `MesosTest` or in `test::Cluster`.  However, in this case, 
the containerizer is allocated via `MesosContainerizer::create` and then passed 
to `MockSlave`.  `MockSlave` does not clean up the containerizer upon 
destruction.

Note: `MockSlave` normally takes a reference to a containerizer allocated on 
the stack, hence why it does not deallocate.


Diffs
-

  src/tests/slave_tests.cpp 7c9dcc6186a8cccb0eb30ff59914a41961e47293 

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


Testing (updated)
---

`make check`

Note: If you **only** add `delete containerizer.get();` to the end of the test, 
the test suite will output:
```
[--] Global test environment tear-down
../../src/tests/environment.cpp:488: Failure
Failed
Tests completed with child processes remaining:
-+- 29543 /path/to/mesos/build/src/.libs/mesos-tests
 --- 29558 /bin/sh /path/to/mesos/build/src/mesos-containerizer launch 
--command={"shell":true,"value":"\/path\/to\/mesos\/build\/src\/mesos-executor"}
 --commands={"commands":[]} --directory=/tmp --help=false --pipe_read=13 
--pipe_write=16 --user=test
```


Thanks,

Joseph Wu

Re: Review Request 39415: Error out when root qdisc already exists

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Nov. 19, 2015, 8:08 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39415/
> ---
> 
> (Updated Nov. 19, 2015, 8:08 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we enable --egress_unique_flow_per_container, we need to install an 
> fq_codel qdisc on root, but if a qdisc already exists on root, we should not 
> continue since it could be not fq_codel at all, so just exit with error.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> cc0152ce44e9fa34210efda8a1c4a6374167eab1 
> 
> Diff: https://reviews.apache.org/r/39415/diff/
> 
> 
> Testing
> ---
> 
> Manual tests, with a pre-installed HTB qdisc and without.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Review Request 40507: Cleanup leaked containerizer and potentially orphaned process in SlaveTest.LaunchTaskInfoWithContainerInfo.

[Joseph Wu]

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

Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.


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


Repository: mesos


Description
---

Normally, the containerizer would be cleaned and deallocated by one of the 
helper methods in `MesosTest` or in `test::Cluster`.  However, in this case, 
the containerizer is allocated via `MesosContainerizer::create` and then passed 
to `MockSlave`.  `MockSlave` does not clean up the containerizer upon 
destruction.

Note: `MockSlave` normally takes a reference to a containerizer allocated on 
the stack, hence why it does not deallocate.


Diffs
-

  src/tests/slave_tests.cpp 7c9dcc6186a8cccb0eb30ff59914a41961e47293 

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


Testing
---

`make check`


Thanks,

Joseph Wu

Re: Review Request 39988: [4/5] Added authorization for dynamic reservation master endpoints.

[Jie Yu]

> On Nov. 13, 2015, 11:38 p.m., Jie Yu wrote:
> > src/master/http.cpp, line 853
> > 
> >
> > This looks problematic to me. 'this' will become invalid once this 
> > function returns. That means when `_reserve` is actually called after the 
> > authorization is done, it might be a dangling pointer.
> > 
> > Instead, you can try to copy the http object (something like below):
> > 
> > ```
> > lambda::function(bool)> _reserve =
> >   lambda::bind(
> >   &Master::Http::_operation,
> >   *this,
> >   slaveId,
> >   resources.flatten(),
> >   operation,
> >   lambda::_1);
> > 
> > return master->authorizeReserveResources(operation.reserve(), principal)
> >   .then(defer(master->self(), _reserve));
> > ```
> 
> Greg Mann wrote:
> Good catch, thanks!

Mpark reached out to me about this issue. Seems that this extra copying is not 
necessary (my bad). The 'http' object will be bounded in the lambda function 
and stored in Master's libprocess handlers map.


- Jie


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


On Nov. 16, 2015, 11:53 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39988/
> ---
> 
> (Updated Nov. 16, 2015, 11:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization for dynamic reservation master endpoints.
> Note: this review is continued from https://reviews.apache.org/r/37126/
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
> 
> Diff: https://reviews.apache.org/r/39988/diff/
> 
> 
> Testing
> ---
> 
> This is the fourth in a chain of 5 patches. Added new reservation endpoints 
> tests to validate authorization of reserve and unreserve operations using 
> ACLs. `make check` was run to test after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Alexander Rukletsov]

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



src/tests/master_quota_tests.cpp 


Could you please restore this blank line?


- Alexander Rukletsov


On Nov. 19, 2015, 7:10 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40392/
> ---
> 
> (Updated Nov. 19, 2015, 7:10 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3911
> https://issues.apache.org/jira/browse/MESOS-3911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added force flag to override quota capacityHeuristic check.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40392/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 40506: Add stdout/tests/numify_tests.cpp into Makefile.am

[Cong Wang]

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

Review request for mesos, Ben Mahler and Ian Downes.


Repository: mesos


Description
---

Add stdout/tests/numify_tests.cpp into Makefile.am


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
0adbe539afaf683e4a85582463a2930049a63998 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Cong Wang]

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

(Updated Nov. 19, 2015, 10:59 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Rebase


Repository: mesos


Description
---

When --egress_unique_flow_per_container is enabled, we need to install a flow 
classifier (fq_codel) qdisc on egress side. This flag specifies where to 
install it in the hierarchy. By default, we install it at root. But, for 
example, if you have already installed HTB qdisc at root, you may want this to 
be installed in other place than root, specify an HTB class ID as its parent 
here.


Diffs (updated)
-

  docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
cc0152ce44e9fa34210efda8a1c4a6374167eab1 
  src/slave/flags.hpp 6ae7c94d2e05d81c8b970e7dcaa82d8aa4de7936 
  src/slave/flags.cpp 26f554e1a73f1f7f3d7baec7bfc1a7f456c5677c 

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


Testing
---

Manual tests, with and without a pre-installed HTB qdisc and classes.


Thanks,

Cong Wang

Re: Review Request 40497: Add hex number support to numify()

[Cong Wang]

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

(Updated Nov. 19, 2015, 10:56 p.m.)


Review request for mesos, Ben Mahler and Ian Downes.


Changes
---

split the patch


Repository: mesos


Description
---

numify() in libprocess uses boost lexical_cast() to cast numbers, but it can 
not cast a hex number. This patch adds hex support to numify().


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
5a0ffc38d4194b9f9c53dc1cf0c90ca7bbae2afd 
  3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
ddd3dd9e015c583e04d72ac9a9b5a5ed6f1d49d5 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
14fb644b38a5cbb8cde74aab39e84305f6ab7041 
  3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.
> 
> Greg Mann wrote:
> FYI, I'm having trouble with the HTTP endpoint callbacks 
> `Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
> things like the following to get `Master::authorizeReserveResources()` to 
> pass the full `Future` return value to `_operation()` so that the 
> validation error can be caught there:
> 
> ```cpp
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(),
>   _operation,
>   slaveId,
>   resources,
>   operation,
>   lambda::_1));
> ```
> 
> 
> or
> 
> 
> ```cpp
> lambda::function(Future)> _reserve =
>   lambda::bind(
>   &Master::Http::_operation,
>   *this,
>   slaveId,
>   resources.flatten(),
>   operation,
>   lambda::_1);
> 
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(), _reserve))
> ```
> 
> 
> where `_operation` is now accepting the entire offer operation as well as 
> a third parameter of type `Framework*` so that the operation can be dropped 
> if necessary.
> 
> Greg Mann wrote:
> But getting errors in the instantiation of `defer()` in the former case, 
> or `lambda::bind()` in the latter...

Thanks Jie! It looks like `.onAny()` only accepts functions with a `void` 
return type:

```
typedef lambda::function&)> AnyCallback;

...

const Future& onAny(AnyCallback&& callback) const;
```


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.
> 
> Greg Mann wrote:
> FYI, I'm having trouble with the HTTP endpoint callbacks 
> `Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
> things like the following to get `Master::authorizeReserveResources()` to 
> pass the full `Future` return value to `_operation()` so that the 
> validation error can be caught there:
> 
> ```cpp
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(),
>   _operation,
>   slaveId,
>   resources,
>   operation,
>   lambda::_1));
> ```
> 
> 
> or
> 
> 
> ```cpp
> lambda::function(Future)> _reserve =
>   lambda::bind(
>   &Master::Http::_operation,
>   *this,
>   slaveId,
>   resources.flatten(),
>   operation,
>   lambda::_1);
> 
> return master->authorizeUnreserveResources(operation, principal, NULL)
>   .onAny(defer(master->self(), _reserve))
> ```
> 
> 
> where `_operation` is now accepting the entire offer operation as well as 
> a third parameter of type `Framework*` so that the operation can be dropped 
> if necessary.

But getting errors in the instantiation of `defer()` in the former case, or 
`lambda::bind()` in the latter...


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 39989: [5/5] Added framework authorization for dynamic reservation.

[Greg Mann]

> On Nov. 14, 2015, 12:07 a.m., Jie Yu wrote:
> > src/master/master.cpp, lines 3037-3049
> > 
> >
> > Hum, this looks problematic to me. The authorization results are stored 
> > in 'futures'. The ordering in 'futures' is important because we'll read the 
> > authorization results in `_accept` based on the ordering.
> > 
> > However, you call `drop` here and skip the authorization. That means 
> > the ordering invariant is no longer hold.
> > 
> > I would suggest that you perform operation validation in 
> > authorizeReserveResources and returns a Failure if validation fails. In 
> > that way, you can drop the operation in `_accept` if authorization returns 
> > a failure.
> > 
> > Please also add a comment about the fact that ordering in 'futures' is 
> > very important.
> > 
> > Also, you may want to add a test to test the cases where RESERVE and 
> > LAUNCH are in one single `accept` call.
> 
> Greg Mann wrote:
> Thanks Jie, this is indeed a problem. I've implemented a solution which 
> simply pushes a failed `Future` onto `futures` so that `_accept()` can handle 
> the failure. The current diff includes this, as well as a new test that 
> explicitly probes the previous bug.
> 
> I like your idea of putting the validation into 
> `authorizeReserveResources()`, especially since similar validation logic is 
> also found in the HTTP endpoint authorization code. That requires a bit of 
> refactoring, which I'm working on currently. Will post a new patch soon.

FYI, I'm having trouble with the HTTP endpoint callbacks 
`Master::Http::reserve()` and `Master::Http::unreserve()`, where I'm trying 
things like the following to get `Master::authorizeReserveResources()` to pass 
the full `Future` return value to `_operation()` so that the validation 
error can be caught there:

```cpp
return master->authorizeUnreserveResources(operation, principal, NULL)
  .onAny(defer(master->self(),
  _operation,
  slaveId,
  resources,
  operation,
  lambda::_1));
```


or


```cpp
lambda::function(Future)> _reserve =
  lambda::bind(
  &Master::Http::_operation,
  *this,
  slaveId,
  resources.flatten(),
  operation,
  lambda::_1);

return master->authorizeUnreserveResources(operation, principal, NULL)
  .onAny(defer(master->self(), _reserve))
```


where `_operation` is now accepting the entire offer operation as well as a 
third parameter of type `Framework*` so that the operation can be dropped if 
necessary.


- Greg


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


On Nov. 17, 2015, 5:07 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39989/
> ---
> 
> (Updated Nov. 17, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, Michael Park, and Till Toenshoff.
> 
> 
> Bugs: MESOS-3062
> https://issues.apache.org/jira/browse/MESOS-3062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added framework authorization for dynamic reservation.
> Note: this review is continued from https://reviews.apache.org/r/37127/
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
> 
> Diff: https://reviews.apache.org/r/39989/diff/
> 
> 
> Testing
> ---
> 
> This is the fifth in a chain of 5 patches. New reservation tests were added 
> to `reservation_tests.cpp` to validate the authentication of framework 
> reserve and unreserve operations using ACLs. `make check` was run to test 
> after all patches were applied.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 40464: Fixed a few style issues in HDFS wrapper code.

[Jie Yu]

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

(Updated Nov. 19, 2015, 10:16 p.m.)


Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


Changes
---

Removed a few unnecessary temp variable.


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


Repository: mesos


Description
---

Fixed a few style issues in HDFS wrapper code.


Diffs (updated)
-

  src/hdfs/hdfs.hpp 6604938e8ca1962db1f0159d175f52fd5c03dd3c 
  src/hdfs/hdfs.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40497]

Failed command: ./support/apply-review.sh -n -r 40497

Error:
 2015-11-19 21:53:08 URL:https://reviews.apache.org/r/40497/diff/raw/ 
[4367/4367] -> "40497.patch" [1]
Successfully applied: Add hex number support to numify()

numify() in libprocess uses boost lexical_cast() to cast numbers, but it can 
not cast a hex number. This patch adds hex support to numify().


Review: https://reviews.apache.org/r/40497
Checking 2 files using filter 
--filter=-,+build/class,+build/deprecated,+build/endif_comment,+readability/todo,+readability/namespace,+runtime/vlog,+whitespace/blank_line,+whitespace/comma,+whitespace/end_of_line,+whitespace/ending_newline,+whitespace/forcolon,+whitespace/indent,+whitespace/line_length,+whitespace/operators,+whitespace/semicolon,+whitespace/tab,+whitespace/todo
Total errors found: 0
ERROR: Commit spanning multiple projects.

Please use separate commits for mesos, libprocess and stout.

Paths grouped by project:
stout:
  3rdparty/libprocess/3rdparty/stout/Makefile.am
  3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt
  3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp
libprocess:
  3rdparty/libprocess/3rdparty/Makefile.am
Failed to commit patch

- Mesos ReviewBot


On Nov. 19, 2015, 8:15 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39417/
> ---
> 
> (Updated Nov. 19, 2015, 8:15 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When --egress_unique_flow_per_container is enabled, we need to install a flow 
> classifier (fq_codel) qdisc on egress side. This flag specifies where to 
> install it in the hierarchy. By default, we install it at root. But, for 
> example, if you have already installed HTB qdisc at root, you may want this 
> to be installed in other place than root, specify an HTB class ID as its 
> parent here.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> cc0152ce44e9fa34210efda8a1c4a6374167eab1 
>   src/slave/flags.hpp 6ae7c94d2e05d81c8b970e7dcaa82d8aa4de7936 
>   src/slave/flags.cpp 26f554e1a73f1f7f3d7baec7bfc1a7f456c5677c 
> 
> Diff: https://reviews.apache.org/r/39417/diff/
> 
> 
> Testing
> ---
> 
> Manual tests, with and without a pre-installed HTB qdisc and classes.
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Review Request 40501: Cleanup a leaked reference to a test process living in the stack.

[Joseph Wu]

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

Review request for mesos, Bernd Mathiske, Artem Harutyunyan, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

Two of the fetcher tests will spawn a process which is stored in the stack 
(i.e. local variable in the test).  `spawn` will store a pointer to the process 
in libprocess's `ProcessManager`.  When the test finishes, the process goes out 
of scope and is therefore lost.  However, the process is **not** terminated.

Failing to terminate this process will lead to an infinite loop in 
`~ProcessManager`, which is called in `process::finalize`.  In `ProcessManager` 
's destructor, we will loop and try to kill all processes.  The process spawned 
in the test will be running.  However, since the pointer lives in the stack, 
the `ProcessManager` will be unable to find the process and will thereby be 
stuck trying to kill a process it cannot find.


Diffs
-

  src/tests/fetcher_tests.cpp 04079964b3539f555351d1444f3635c64700a1a8 

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


Testing
---

`make check`

Additional testing:

Insert a `process::finalize` in `src/test/main.cpp`.  i.e.
```
  // Replace `return RUN_ALL_TESTS();` with this:
  int ret = RUN_ALL_TESTS();
  process::finalize();
  return ret;
```

Then `make check 
GTEST_FILTER="*FetcherTest.OSNetUriTest*:*FetcherTest.OSNetUriSpaceTest*"`.
The test program should not stall or segfault or abort in some weird way.


Thanks,

Joseph Wu

Re: Review Request 40268: [WIP] Libprocess Reinitialization: Change Socket::DEFAULT_KIND to return a non-static local value.

[Joseph Wu]

> On Nov. 19, 2015, 1:33 p.m., Vinod Kone wrote:
> > i'm discarding this review to unblock review bot -- @vinodkone

Oops, I didn't know that reviewbot would get stuck on a (dangling?) review like 
this :)


- Joseph


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


On Nov. 18, 2015, 1:52 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40268/
> ---
> 
> (Updated Nov. 18, 2015, 1:52 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is only needed for tests that utilize the test-only 
> `process::reinitialize` function.
> 
> Note: This is part of a review chain draft, so ignore any links in "Depends 
> On" or "Blocks".  This commit revealed some incomplete cleanup in tests, 
> which is fixed here:
> https://reviews.apache.org/r/40453/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ebee78909feb5a4032da68f51d08dbf11b03b332 
>   3rdparty/libprocess/src/socket.cpp 6ac834e7459f5958b7c788ccdc60cbed90530183 
> 
> Diff: https://reviews.apache.org/r/40268/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40268: [WIP] Libprocess Reinitialization: Change Socket::DEFAULT_KIND to return a non-static local value.

[Vinod Kone]

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


i'm discarding this review to unblock review bot -- @vinodkone

- Vinod Kone


On Nov. 18, 2015, 9:52 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40268/
> ---
> 
> (Updated Nov. 18, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3820
> https://issues.apache.org/jira/browse/MESOS-3820
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is only needed for tests that utilize the test-only 
> `process::reinitialize` function.
> 
> Note: This is part of a review chain draft, so ignore any links in "Depends 
> On" or "Blocks".  This commit revealed some incomplete cleanup in tests, 
> which is fixed here:
> https://reviews.apache.org/r/40453/
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> ebee78909feb5a4032da68f51d08dbf11b03b332 
>   3rdparty/libprocess/src/socket.cpp 6ac834e7459f5958b7c788ccdc60cbed90530183 
> 
> Diff: https://reviews.apache.org/r/40268/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 39712: Serialize Docker Registry Responses as Protobuf

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38901, 38919, 39712]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 19, 2015, 7:37 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39712/
> ---
> 
> (Updated Nov. 19, 2015, 7:37 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3514
> https://issues.apache.org/jira/browse/MESOS-3514
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Serialize Docker Registry Responses as Protobuf
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
> 3fd84e380f9393a6d46544fb7fc7d24bcb6256ac 
>   src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
> b17b1b6d9e053af8c80217ea322e99ccab957df1 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 7fca99a3b118b0ac06566455ef003daeef7d8157 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 9ff27083be91985484960a0d48d909b5594937db 
> 
> Diff: https://reviews.apache.org/r/39712/diff/
> 
> 
> Testing
> ---
> 
> make check(ubuntu14.04 + clang-3.6)
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 40242: Improved docs for dynamic reservation HTTP endpoints.

[Neil Conway]

> On Nov. 19, 2015, 2:44 a.m., Guangya Liu wrote:
> > docs/reservation.md, line 242
> > 
> >
> > This will not work if end user did not enable autheration, a JIRA 
> > ticket is tracing this MESOS-3940, shall we highlight the issue here before 
> > get resolved?

I think this is okay -- the issue should be fixed shortly.


- Neil


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


On Nov. 18, 2015, 11:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40242/
> ---
> 
> (Updated Nov. 18, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Bugs: MESOS-2455
> https://issues.apache.org/jira/browse/MESOS-2455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved docs for dynamic reservation HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   docs/home.md 7aa785e9ae07f2cc14eb0f1108ae4ab4c8748599 
>   docs/persistent-volume.md 0951ccb69daaa19b959e11cf3bf972a674a58305 
>   docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a 
> 
> Diff: https://reviews.apache.org/r/40242/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Review Request 40498: Used factory method to create HDFS client.

[Jie Yu]

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

Review request for mesos, Bernd Mathiske, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Used factory method to create HDFS client.


Diffs
-

  src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc 
  src/hdfs/hdfs.hpp 6604938e8ca1962db1f0159d175f52fd5c03dd3c 
  src/hdfs/hdfs.cpp PRE-CREATION 
  src/launcher/fetcher.cpp 7ad3206c31c47b7d1ae9f89c9e9347831aa3ea39 
  src/slave/containerizer/fetcher.cpp 0b676f43e8529c6790824c8cf82e21a81d885ef1 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 39417: Add --egress_flow_classifier_parent flag

[Cong Wang]

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

(Updated Nov. 19, 2015, 8:15 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Use numify()


Repository: mesos


Description
---

When --egress_unique_flow_per_container is enabled, we need to install a flow 
classifier (fq_codel) qdisc on egress side. This flag specifies where to 
install it in the hierarchy. By default, we install it at root. But, for 
example, if you have already installed HTB qdisc at root, you may want this to 
be installed in other place than root, specify an HTB class ID as its parent 
here.


Diffs (updated)
-

  docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
cc0152ce44e9fa34210efda8a1c4a6374167eab1 
  src/slave/flags.hpp 6ae7c94d2e05d81c8b970e7dcaa82d8aa4de7936 
  src/slave/flags.cpp 26f554e1a73f1f7f3d7baec7bfc1a7f456c5677c 

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


Testing
---

Manual tests, with and without a pre-installed HTB qdisc and classes.


Thanks,

Cong Wang

Review Request 40497: Add hex number support to numify()

[Cong Wang]

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

Review request for mesos, Ben Mahler and Ian Downes.


Repository: mesos


Description
---

numify() in libprocess uses boost lexical_cast() to cast numbers, but it can 
not cast a hex number. This patch adds hex support to numify().


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
0adbe539afaf683e4a85582463a2930049a63998 
  3rdparty/libprocess/3rdparty/stout/Makefile.am 
5a0ffc38d4194b9f9c53dc1cf0c90ca7bbae2afd 
  3rdparty/libprocess/3rdparty/stout/include/stout/numify.hpp 
ddd3dd9e015c583e04d72ac9a9b5a5ed6f1d49d5 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
14fb644b38a5cbb8cde74aab39e84305f6ab7041 
  3rdparty/libprocess/3rdparty/stout/tests/numify_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 39415: Error out when root qdisc already exists

[Cong Wang]

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

(Updated Nov. 19, 2015, 8:08 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Rebase


Repository: mesos


Description
---

When we enable --egress_unique_flow_per_container, we need to install an 
fq_codel qdisc on root, but if a qdisc already exists on root, we should not 
continue since it could be not fq_codel at all, so just exit with error.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
cc0152ce44e9fa34210efda8a1c4a6374167eab1 

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


Testing
---

Manual tests, with a pre-installed HTB qdisc and without.


Thanks,

Cong Wang

Re: Review Request 39416: Document --egress_unique_flow_per_container in docs/configuration.md

[Cong Wang]

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

(Updated Nov. 19, 2015, 8:09 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Rebase


Repository: mesos


Description
---

Flag --egress_unique_flow_per_container is documented in help message, but not 
in docs/configuration.md. We should document it there too, with more details.


Diffs (updated)
-

  docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 

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


Testing
---

None.


Thanks,

Cong Wang

Re: Review Request 40459: Added 2 slave flags --advertise_ip and --advertise_port.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On Nov. 19, 2015, 12:58 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40459/
> ---
> 
> (Updated Nov. 19, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Timothy Chen.
> 
> 
> Bugs: MESOS-3809
> https://issues.apache.org/jira/browse/MESOS-3809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If set, these IP/Port shall be advertised by libprocess (although bind is not 
> done on this IP/Port). If not set, libprocess advertises the IP/Port on which 
> bind was done.
> 
> Command line arguments added:
> advertise_ip: IP address advertised to reach mesos slave.
> advertise_port: Port advertised to reach mesos slave (used with advertise_ip).
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
>   src/slave/main.cpp 746eec684a0c9119f85506022676d276f808b6ea 
> 
> Diff: https://reviews.apache.org/r/40459/diff/
> 
> 
> Testing
> ---
> 
> make check run successfully.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 39712: Serialize Docker Registry Responses as Protobuf

[Gilbert Song]

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

(Updated Nov. 19, 2015, 11:37 a.m.)


Review request for mesos, Jie Yu, Jojy Varghese, and Timothy Chen.


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


Repository: mesos


Description
---

Serialize Docker Registry Responses as Protobuf


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/registry_client.hpp 
3fd84e380f9393a6d46544fb7fc7d24bcb6256ac 
  src/slave/containerizer/mesos/provisioner/docker/registry_client.cpp 
b17b1b6d9e053af8c80217ea322e99ccab957df1 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
7fca99a3b118b0ac06566455ef003daeef7d8157 
  src/tests/containerizer/provisioner_docker_tests.cpp 
9ff27083be91985484960a0d48d909b5594937db 

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


Testing
---

make check(ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 40469: Update Allocator interface to support dynamic roles

[Neil Conway]

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



include/mesos/master/allocator.hpp (line 401)


Second sentence seems redundant with the first.



include/mesos/master/allocator.hpp (line 409)


Redundant.


- Neil Conway


On Nov. 19, 2015, 6:54 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40469/
> ---
> 
> (Updated Nov. 19, 2015, 6:54 a.m.)
> 
> 
> Review request for mesos, Adam B, Guangya Liu, and Qian Zhang.
> 
> 
> Bugs: MESOS-3956
> https://issues.apache.org/jira/browse/MESOS-3956
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Update Allocator interface to support dynamic roles
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
>   src/master/allocator/mesos/allocator.hpp 
> d2d32af227d66c4030becd4cd64b907a70d25f49 
>   src/master/allocator/mesos/hierarchical.hpp 
> 64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
>   src/master/allocator/mesos/hierarchical.cpp 
> f2e3b639f210eb06c70584ee7294609d9fd978ad 
>   src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
> 
> Diff: https://reviews.apache.org/r/40469/diff/
> 
> 
> Testing
> ---
> 
> Make check successfully.
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Joerg Schad]

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

(Updated Nov. 19, 2015, 7:10 p.m.)


Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.


Changes
---

Removed rebase artifact.


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


Repository: mesos


Description
---

Added force flag to override quota capacityHeuristic check.


Diffs (updated)
-

  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Joerg Schad

Re: Review Request 40247: Added HTTP endpoints for creating and destroying persistent volumes.

[Michael Park]

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



docs/persistent-volume.md (lines 255 - 280)


This looks to be formatted weird, could you double check? Here and below



src/master/http.cpp (line 612)


The `volumes.flatten()` here looks incorrect to me. For `/reserve`, we call 
`flatten` since the `required` resources are the same resources in their 
unreserved state. For `/create-volume` however, I think the `required` 
resources are the same resources with no `DiskInfo` information.

For example, suppose we're requested to perform a `CREATE` operation with a 
4GB volume statically reserved for role R. The resources we're searching for is 
4GB disk statically reserved for role R. As opposed to 4GB volume unreserved, 
which is what `flatten()` gives us.


- Michael Park


On Nov. 18, 2015, 11:28 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40247/
> ---
> 
> (Updated Nov. 18, 2015, 11:28 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Bugs: MESOS-2455
> https://issues.apache.org/jira/browse/MESOS-2455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added HTTP endpoints for creating and destroying persistent volumes.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 0951ccb69daaa19b959e11cf3bf972a674a58305 
>   docs/reservation.md 81f21c3755b216b0932876c1ddd9de4d3fbe814a 
>   src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/master.cpp 39a0e730b230cee73e30d831ee67d9207359ae28 
>   src/tests/mesos.hpp b3f69ccb9870b17a335a2fe7dbf2802c1b709e6b 
>   src/tests/persistent_volume_endpoints_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40247/diff/
> 
> 
> Testing
> ---
> 
> (1) make check, including newly added tests
> 
> (2) Manually created/removed persistent volumes via HTTP endpoints + curl.
> 
> (3) Previewed docs in Github gist.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 40291: Added the stye guideline for blank line after line-wrapping.

[Neil Conway]

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

Ship it!


We might want to clarify that if you have a multi-line statement that is 
immediately followed by a closing brace, you don't need a newline before the 
brace.


docs/c++-style-guide.md (line 114)


Why the <> in  ?



docs/c++-style-guide.md (line 118)


The point is that the for loop condition should be defined over multiple 
lines, right?


- Neil Conway


On Nov. 16, 2015, 5:32 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40291/
> ---
> 
> (Updated Nov. 16, 2015, 5:32 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the stye guideline for blank line after line-wrapping.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
> 
> Diff: https://reviews.apache.org/r/40291/diff/
> 
> 
> Testing
> ---
> 
> viewed rendered doc.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40459: Added 2 slave flags --advertise_ip and --advertise_port.

[Anindya Sinha]

> On Nov. 19, 2015, 2:57 a.m., Guangya Liu wrote:
> > docs/configuration.md, lines 37-54
> > 
> >
> > I think that the master/main.cpp should also be updated for those 
> > flages.
> 
> Anindya Sinha wrote:
> It is already there added as a part of MESOS-809. Refer 
> https://github.com/apache/mesos/blob/master/src/master/main.cpp#L131 and 
> https://github.com/apache/mesos/blob/master/src/master/main.cpp#L138.
> 
> Guangya Liu wrote:
> I mean this part should also be updated to use the latest description as 
> here you have updated "mesos master" to "mesos master/slave"

Sorry, I am not sure I understand the concern. Are you suggesting we should 
have the exact same text when we do "mesos-master --help" or "mesos-slave 
--help" or look in the documentation.

In docs/configuration.md, I moved description of advertise_ip and 
advertise_port to the section of "Master and Slave Options" from the original 
"Master options" to denote the flags correspond to both master as well as slave 
(and the text reflects that). And it uses "mesos master/slave" since this is a 
common command line arg.
In src/slave/main.cpp: It has the same language as in docs/configuration.md 
except that I mention "slave" instead of "master/slave".
In src/master/main.cpp: It has the same language as in docs/configuration.md 
except that I mention "master" instead of "master/slave".

So when we do "mesos-master --help", we see mesos master (not mesos 
master/slave).
And when we do "mesos-slave --help", we see mesos slave (not mesos 
master/slave).
But in documentation, we see mesos master/slave (since these 2 flags are common 
to both mesos master as well as mesos slave).


- Anindya


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


On Nov. 19, 2015, 12:58 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40459/
> ---
> 
> (Updated Nov. 19, 2015, 12:58 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Timothy Chen.
> 
> 
> Bugs: MESOS-3809
> https://issues.apache.org/jira/browse/MESOS-3809
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If set, these IP/Port shall be advertised by libprocess (although bind is not 
> done on this IP/Port). If not set, libprocess advertises the IP/Port on which 
> bind was done.
> 
> Command line arguments added:
> advertise_ip: IP address advertised to reach mesos slave.
> advertise_port: Port advertised to reach mesos slave (used with advertise_ip).
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 72847e5efe7008fdec8287cce100857f9e7c0fe0 
>   src/slave/main.cpp 746eec684a0c9119f85506022676d276f808b6ea 
> 
> Diff: https://reviews.apache.org/r/40459/diff/
> 
> 
> Testing
> ---
> 
> make check run successfully.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 40305: Added URI fetcher interface.

[Jie Yu]

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

(Updated Nov. 19, 2015, 6:30 p.m.)


Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone.


Changes
---

Added the missing virtual destructor.


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


Repository: mesos


Description
---

Added URI fetcher interface.


Diffs (updated)
-

  include/mesos/uri/fetcher.hpp PRE-CREATION 
  src/CMakeLists.txt 9a2c70d40031c80a304462107758802c08411a49 
  src/Makefile.am 5aac0ee8657dfca17a232438fd561cb00fd5e752 
  src/uri/fetcher.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Alexander Rukletsov]

> On Nov. 18, 2015, 5:08 p.m., Joerg Schad wrote:
> > src/master/quota_handler.cpp, line 224
> > 
> >
> > As discussed offline I would prefere a single json object per request 
> > which is discussed by MESOS-3914. Until then I am ok with having the flag 
> > in the request body.
> 
> Joerg Schad wrote:
> This is an answer to Alex question above.

I still think we should not ship the feature without cleaning this up. Filed: 
https://issues.apache.org/jira/browse/MESOS-3960


- Alexander


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


On Nov. 19, 2015, 9:56 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40392/
> ---
> 
> (Updated Nov. 19, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3911
> https://issues.apache.org/jira/browse/MESOS-3911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added force flag to override quota capacityHeuristic check.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40392/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40392: Added force flag to override quota capacityHeuristic check.

[Alexander Rukletsov]

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

Ship it!



src/tests/master_quota_tests.cpp (lines 458 - 460)


Looks like rebase artifact.


- Alexander Rukletsov


On Nov. 19, 2015, 9:56 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40392/
> ---
> 
> (Updated Nov. 19, 2015, 9:56 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3911
> https://issues.apache.org/jira/browse/MESOS-3911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added force flag to override quota capacityHeuristic check.
> 
> 
> Diffs
> -
> 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/40392/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40285: Changed untar process to pipe STDERR.

[Jojy Varghese]

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

(Updated Nov. 19, 2015, 6:05 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

rebased.


Repository: mesos


Description
---

By piping stderr to logs, it would be easier to  debug problems with untar.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/puller.cpp 
78129a8d306f77403cd99357a1d2a65dc3e65c7b 

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


Testing
---

make check;


Thanks,

Jojy Varghese

Re: Review Request 40339: Added a flag to master to enable optimistic offers.

[Joseph Wu]

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


Overall notes:
* Looks like everything is in place (to my knowledge) for this change :)
* (Mentioned in the last working group sync) We **may** want to rename the 
feature to something else, like "oversubscription for reservations".


include/mesos/master/allocator.hpp (line 86)


s/automatically//



src/master/flags.cpp (line 441)


s/automatically//

Also, we'll want a more in-depth help message.  It should have:
* An very, very brief overview of optimistic offers.
* What the consequences are for enabling it.
* What the consequences are for disabling it.
* The default if not specified.


- Joseph Wu


On Nov. 16, 2015, 1:40 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40339/
> ---
> 
> (Updated Nov. 16, 2015, 1:40 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Joris Van Remoortere, and Klaus 
> Ma.
> 
> 
> Bugs: MESOS-3887
> https://issues.apache.org/jira/browse/MESOS-3887
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag to master to enable optimistic offers.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
>   src/master/allocator/mesos/allocator.hpp 
> d2d32af227d66c4030becd4cd64b907a70d25f49 
>   src/master/allocator/mesos/hierarchical.hpp 
> cd3c7019952bfe7c1049a9c42a90943b681f1939 
>   src/master/allocator/mesos/hierarchical.cpp 
> e9286260f490915327f44492086c1160d5567ac5 
>   src/master/flags.hpp 5fd5d502697b2edc22ae98a5a8e361bf62bf8bb6 
>   src/master/flags.cpp 806e2da6ad37a6acf76818d4c6b3c462175fd09d 
>   src/master/master.cpp 7bac0fea4bcd040307fdfdcd002387d5baee46d1 
>   src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
>   src/tests/hierarchical_allocator_tests.cpp 
> 740cfa801ee90417c038308192d1f4f2416f8315 
>   src/tests/master_allocator_tests.cpp 
> 646cacd3e16b9e0b72c0b259eecf2760cfb530db 
>   src/tests/reservation_endpoints_tests.cpp 
> 1552e4537c4f4d79bfa4bc17ccab2df630bc32a4 
>   src/tests/reservation_tests.cpp ac664ebb49e74aa28551f427ea8f39ac9ce0cfb3 
>   src/tests/resource_offers_tests.cpp 
> bf2fe3ac7b982e31d289a703ae637d8e2b3a2d8a 
>   src/tests/slave_recovery_tests.cpp 2cc7132deb9b8c324aa9dbab0b81643d07377a89 
> 
> Diff: https://reviews.apache.org/r/40339/diff/
> 
> 
> Testing
> ---
> 
> Ubuntu 14.04
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 40398: Quota: Extended allocator interface with recovery method.

[Joerg Schad]

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


Do we have this added to the design doc (or Jira ticket), it would be nice to 
capture some more details about the target functionality.


include/mesos/master/allocator.hpp (line 120)


I assume string is for role? 
if yes:
a) this is non-trivial to guess...
b) why do we need it? isn't that included in QuotaInfo already?


- Joerg Schad


On Nov. 17, 2015, 8:54 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40398/
> ---
> 
> (Updated Nov. 17, 2015, 8:54 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3873
> https://issues.apache.org/jira/browse/MESOS-3873
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp f76118bbf028610c330cebe937d81457fc67a6f3 
>   src/master/allocator/mesos/allocator.hpp 
> d2d32af227d66c4030becd4cd64b907a70d25f49 
>   src/master/allocator/mesos/hierarchical.hpp 
> 64ccf4164197e59d93d739fa2afbdee2cc2a1d23 
>   src/master/allocator/mesos/hierarchical.cpp 
> f2e3b639f210eb06c70584ee7294609d9fd978ad 
>   src/tests/allocator.hpp e0cb2e75e6cac41ae8d8ed1608f1d64e7c533e34 
> 
> Diff: https://reviews.apache.org/r/40398/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40244: Clarified comments in Master::Http::_operation.

[Michael Park]

> On Nov. 18, 2015, 8:05 a.m., Qian Zhang wrote:
> > src/master/http.cpp, line 2097
> > 
> >
> > This comment may not be related to this patch. I am just curious, for 
> > ```required```, we have called ```flatten()``` to remove role from it when 
> > passing it into ```Master::Http::_operation()```. But here for 
> > ```offer->resources()```, we do not call ```flatten()``` to remove its 
> > role, so is it subtractable between ```required``` and 
> > ```offer->resources()```?

We `flatten` the resources for `/reserve` because the `required` represents the 
resources that we need in order to perform the reservation.
Suppose we want to end up with 2 reserved cpus. We `flatten` it, and search for 
2 unreserved cpus in the received offers.
If we were to `flatten` the `offer.resources()` also, then an offer with 1 
reserved cpu and 1 unreserved cpu would incorrectly satisfy our condition.

In terms of subtractibility, that is determined on a per-resource basis, and 
non-subtractable resources are ignored.
For example, `[(2 unreserved cpus)] - [(1 reserved cpu), (1 unreserved cpu)]` 
== `[(1 unreserved cpu)]`.


- Michael


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


On Nov. 18, 2015, 11:27 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40244/
> ---
> 
> (Updated Nov. 18, 2015, 11:27 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Bugs: MESOS-2455
> https://issues.apache.org/jira/browse/MESOS-2455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified comments in Master::Http::_operation.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1c4f1406f5d917f5d655a7d61d311365f8999ce0 
> 
> Diff: https://reviews.apache.org/r/40244/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 39400: Quota: Implemented quota API.

[Joerg Schad]

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



src/master/allocator/mesos/hierarchical.hpp (line 390)


s/separately/ in addition 

Not really sure about the best formulation here but for me 'separately' 
implies that it is not part of the normal sorter anymore.



src/master/allocator/mesos/hierarchical.cpp (line 140)


s/of/for 

similar issue with separately as above.



src/master/allocator/mesos/hierarchical.cpp (line 141)


s/dedicated/additional



src/master/allocator/mesos/hierarchical.cpp (line 142)


s/general/all ?



src/master/allocator/mesos/hierarchical.cpp (line 332)


s/this/these (or all)?



src/master/allocator/mesos/hierarchical.cpp (line 889)


s/is not set/has not been set before



src/master/allocator/mesos/hierarchical.cpp (line 903)


s/an/the



src/master/allocator/mesos/hierarchical.cpp (line 926)


s/an/the


- Joerg Schad


On Nov. 11, 2015, 10:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39400/
> ---
> 
> (Updated Nov. 11, 2015, 10:29 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3718
> https://issues.apache.org/jira/browse/MESOS-3718
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> cd0d3e0bd234fcf4db9af2e376ea311937204f75 
>   src/master/allocator/mesos/hierarchical.cpp 
> 14fef63714721fcda7cea3c28704766efda6d007 
> 
> Diff: https://reviews.apache.org/r/39400/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 40342, 
40351, 38956, 40396, 39223, 39492, 39614]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 19, 2015, 4:05 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.

[Alexander Rukletsov]

> On Nov. 17, 2015, 6:16 a.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 211
> > 
> >
> > For ```offer->resources()```, before doing the math, do we need to call 
> > ```flatten()``` to remove role too? For dynamic reservation, in 
> > ```Http::_operation()```, I see we do not call ```flatten()``` for offered 
> > resources too, is it a bug?
> 
> Alexander Rukletsov wrote:
> In this case everything should be fine. IIUC, there is only one reason 
> why `offer->resources()` has non '*' role: it's a statically reserved 
> resource. Quota is orthogonal to static reservations, hence we should not 
> rescind those offers.
> 
> However, I think this check should be removed here. We may rescind more 
> offers than necessary to satisfy remaining resources (because we want to 
> rescind from a certain number of agents). I'll think about it.
> 
> Qian Zhang wrote:
> But I think for the dynamic reserved resource, ```offer->resources()``` 
> also has non * role and also has non-empty ```ReservationInfo```, right?

Here is "cheat sheet" from @MPark:
```
has_reservation && role == "*" : invalid
has_reservation && role != "*": dynamically reserved
!has_reservation && role == "*": unreserved
!has_reservation && role != "*": statically reserved
```

According to it, you're right.


- Alexander


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


On Nov. 19, 2015, 5:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40351/
> ---
> 
> (Updated Nov. 19, 2015, 5:10 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3912
> https://issues.apache.org/jira/browse/MESOS-3912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40351/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40291: Added the stye guideline for blank line after line-wrapping.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Nov. 16, 2015, 9:32 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40291/
> ---
> 
> (Updated Nov. 16, 2015, 9:32 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the stye guideline for blank line after line-wrapping.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
> 
> Diff: https://reviews.apache.org/r/40291/diff/
> 
> 
> Testing
> ---
> 
> viewed rendered doc.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40292: Added style guideline for writing numbers to markdown styleguide.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Nov. 17, 2015, 12:07 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40292/
> ---
> 
> (Updated Nov. 17, 2015, 12:07 a.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added style guideline for writing numbers to markdown styleguide.
> 
> 
> Diffs
> -
> 
>   docs/markdown-style-guide.md fe9ffefbbb2c0ce9ade39cec1de073163a9a03b2 
> 
> Diff: https://reviews.apache.org/r/40292/diff/
> 
> 
> Testing
> ---
> 
> viewed rendered version.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.

[Alexander Rukletsov]

> On Nov. 18, 2015, 7:51 a.m., Qian Zhang wrote:
> > src/master/quota_handler.cpp, line 180
> > 
> >
> > Why do we want to rescind the offeres that do not contribute to 
> > satisfying quota request?

Because we may rescind more than necessary to satisfy quota request (remember 
minimal agent count). If we have a check in place, this will effectively 
prevent us from doing so. Does it make sense to you?


- Alexander


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


On Nov. 19, 2015, 5:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40351/
> ---
> 
> (Updated Nov. 19, 2015, 5:10 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-3912
> https://issues.apache.org/jira/browse/MESOS-3912
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/40351/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS X 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.

[Alexander Rukletsov]

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

(Updated Nov. 19, 2015, 5:10 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Fixed a typo.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40293: Applied consistent number syle in c++ styleguide.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Nov. 16, 2015, 11:58 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40293/
> ---
> 
> (Updated Nov. 16, 2015, 11:58 p.m.)
> 
> 
> Review request for mesos and Bernd Mathiske.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Applied consistent number syle in c++ styleguide.
> 
> 
> Diffs
> -
> 
>   docs/c++-style-guide.md 0b6189174a4f0f1815625f68fb1a743b04a9cdad 
> 
> Diff: https://reviews.apache.org/r/40293/diff/
> 
> 
> Testing
> ---
> 
> viewed rendered version.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40351: Quota: Added rescinding offers for set quota requests.

[Alexander Rukletsov]

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

(Updated Nov. 19, 2015, 5:01 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Added a missing blank line.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40399: Quota: Introduced quota registry operations.

[Alexander Rukletsov]

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

(Updated Nov. 19, 2015, 4:45 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, Joris Van Remoortere, 
and Joseph Wu.


Changes
---

Simplified protobuf manipulation code.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/quota.hpp 8fc984003762a339a796b7a516362e21dab0a6e5 
  src/master/quota.cpp 835b191460d86e912ffdac8160459b4a0643bd88 

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


Testing
---

make check (Mac OS X 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 40371: Changed mesos-execute to add containerizer option.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [40284, 40285]

Failed command: ./support/apply-review.sh -n -r 40285

Error:
 2015-11-19 16:15:38 URL:https://reviews.apache.org/r/40285/diff/raw/ 
[3128/3128] -> "40285.patch" [1]
error: patch failed: 
src/slave/containerizer/mesos/provisioner/docker/puller.cpp:18
error: src/slave/containerizer/mesos/provisioner/docker/puller.cpp: patch does 
not apply
Failed to apply patch

- Mesos ReviewBot


On Nov. 19, 2015, 3:37 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40371/
> ---
> 
> (Updated Nov. 19, 2015, 3:37 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since docker_image option could be used for mesos and docker containerizer,
> introduced a  new option 'containerizer' to disambiguate the two 
> containerizers.
> 
> New usage:
> mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
> --name=test_docker --docker_image=ubuntu --containerizer=docker
> 
> mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
> --name=test_mesos --docker_image=ubuntu --containerizer=mesos
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc 
> 
> Diff: https://reviews.apache.org/r/40371/diff/
> 
> 
> Testing
> ---
> 
> tested the two containerizer locally with simple commands.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 39492: Added status endpoint for quota master endpoint.

[Joerg Schad]

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

(Updated Nov. 19, 2015, 4:06 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

Added status handling for quota master endpoint.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

Tests are in next Review.


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Joerg Schad]

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

(Updated Nov. 19, 2015, 4:05 p.m.)


Review request for mesos, Alexander Rukletsov and Bernd Mathiske.


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


Repository: mesos


Description
---

Quota: Added Status Validation Tests.


Diffs (updated)
-

  src/tests/master_quota_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Joerg Schad]

> On Nov. 19, 2015, 3:34 p.m., Alexander Rukletsov wrote:
> > src/tests/master_quota_tests.cpp, line 521
> > 
> >
> > `protobuf::parse()` does not compile?

No.


- Joerg


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


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [39211, 39018, 39102, 36913, 38059, 39285, 38110, 40342, 
40351, 38956, 40396, 39223, 39492, 39614]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39492: Added status endpoint for quota master endpoint.

[Alexander Rukletsov]

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



src/master/quota_handler.cpp (line 347)


let's `static_cast` to `int` explicitly


- Alexander Rukletsov


On Nov. 19, 2015, 3:19 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39492/
> ---
> 
> (Updated Nov. 19, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added status handling for quota master endpoint.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/39492/diff/
> 
> 
> Testing
> ---
> 
> Tests are in next Review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39492: Added status endpoint for quota master endpoint.

[Alexander Rukletsov]

> On Nov. 19, 2015, 2:53 p.m., Alexander Rukletsov wrote:
> > src/master/quota_handler.cpp, line 338
> > 
> >
> > s/Status/status
> > Do you think it makes sense to add `request.body`.
> 
> Joerg Schad wrote:
> Considered adding request body, but as of right now the body is empty, or?

I'm thinking about the evolution of the request. Maybe it makes sense to add 
URL parameters? Will people be using extra flags? On the other side we can add 
it later. Dropping the issue.


- Alexander


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


On Nov. 19, 2015, 3:19 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39492/
> ---
> 
> (Updated Nov. 19, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added status handling for quota master endpoint.
> 
> 
> Diffs
> -
> 
>   include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
>   src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
>   src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 
> 
> Diff: https://reviews.apache.org/r/39492/diff/
> 
> 
> Testing
> ---
> 
> Tests are in next Review.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 40371: Changed mesos-execute to add containerizer option.

[Jojy Varghese]

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

(Updated Nov. 19, 2015, 3:37 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

added back check for docker image in main.


Repository: mesos


Description (updated)
---

Since docker_image option could be used for mesos and docker containerizer,
introduced a  new option 'containerizer' to disambiguate the two containerizers.

New usage:
mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
--name=test_docker --docker_image=ubuntu --containerizer=docker

mesos-execute --master=127.0.0.1:5050 --command="uname -a" \
--name=test_mesos --docker_image=ubuntu --containerizer=mesos


Diffs (updated)
-

  src/cli/execute.cpp d070164e080cb74ee15d3184487a121f429a29fc 

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


Testing
---

tested the two containerizer locally with simple commands.


Thanks,

Jojy Varghese

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Alexander Rukletsov]

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



src/tests/master_quota_tests.cpp (line 435)


let's wrap "from" to avoid jaggedness.



src/tests/master_quota_tests.cpp (line 443)


Don't we need to authenticate?



src/tests/master_quota_tests.cpp (lines 455 - 456)


It feels like this test may not test what it should. The field may miss for 
two reasons: no quotas set _or_ there is no such field at all : ).

I'd suggest to either convert to the protobuf and then check the size of 
the collection, or check against entire `JSON` object.



src/tests/master_quota_tests.cpp (line 479)


indent



src/tests/master_quota_tests.cpp (lines 479 - 480)


blank line



src/tests/master_quota_tests.cpp (line 490)


space



src/tests/master_quota_tests.cpp (lines 490 - 491)


blank line



src/tests/master_quota_tests.cpp (lines 494 - 497)


Let's add a check that total cluster resources contain `quotaResources`.



src/tests/master_quota_tests.cpp (lines 495 - 496)


Fits one line



src/tests/master_quota_tests.cpp (line 496)


Please use constant `ROLE1`



src/tests/master_quota_tests.cpp (lines 500 - 503)


indentation



src/tests/master_quota_tests.cpp (line 520)


How about "Convert `JSON` response to `QuotaStatus` protobuf"?

Also, mind backticks.



src/tests/master_quota_tests.cpp (line 521)


`protobuf::parse()` does not compile?



src/tests/master_quota_tests.cpp (line 524)


`RepeatedField::size()` returns `int`


- Alexander Rukletsov


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 39492: Added status endpoint for quota master endpoint.

[Joerg Schad]

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

(Updated Nov. 19, 2015, 3:19 p.m.)


Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


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


Repository: mesos


Description
---

Added status handling for quota master endpoint.


Diffs (updated)
-

  include/mesos/quota/quota.proto 4e4d8ccc92e2bf9a8e5eae8488c0c952f82fdd6d 
  src/master/master.hpp 5e5a575dc7dd49324f3c837028df8a7f75cd1f80 
  src/master/quota_handler.cpp 03cef4117c52da7599a2800060f65483ca33bc3f 

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


Testing
---

Tests are in next Review.


Thanks,

Joerg Schad

Re: Review Request 39614: Quota: Added Status Validation Tests.

[Joerg Schad]

> On Nov. 6, 2015, 1:13 a.m., Joseph Wu wrote:
> > src/tests/master_quota_tests.cpp, lines 732-734
> > 
> >
> > Have you considered merging this test with 
> > `AvailableResourcesMultipleAgents`?
> 
> Joerg Schad wrote:
> In my opinion it is good to test both aspects seperatly from each other.
> 
> Alexander Rukletsov wrote:
> I think it's okay to merge as long as it's clear what fails. Same for 
> situations when no quota is set. Moreover, once we have a test with several 
> quotas, we can check status of multiple quotas as well.

I must say that alone from a grouping point I personally prefer to be able to 
easily execute find all Status related tests (and not having to remember that 
AvailableResourcesMultipleAgents also tests some functionality).


- Joerg


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


On Nov. 19, 2015, 2:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39614/
> ---
> 
> (Updated Nov. 19, 2015, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Quota: Added Status Validation Tests.
> 
> 
> Diffs
> -
> 
>   src/tests/master_quota_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/39614/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>