Review Request 42784: Allocator Benchmark: Pull resource parsing out of for loop.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
b1cb955b7eb1213c7ba4a9c5181545bb49154f06 

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


Testing
---


Thanks,

Joris Van Remoortere

Review Request 42789: Added support file to autogenerate docs from endpoint help strings.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

After building mesos from source, an executable called
mesos-endpoint-docs will be created in build/src. Invoke this executable
with the flag --dirname=../docs/endpoints to autogenerate documentation
for all endpoint strings and install it into the proper location.  In a
subsequent commit we will add a link to this directory in home.md.


Diffs
-

  src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
  support/endpoint-docs.cpp PRE-CREATION 

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


Testing
---


Thanks,

Kevin Klues

Review Request 42790: Added documentation for all http endpoints.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

These .md files were auto generated using mesos-endpoints-help.


Diffs
-

  docs/endpoints/files/browse.json/index.md PRE-CREATION 
  docs/endpoints/files/browse.md PRE-CREATION 
  docs/endpoints/files/debug.json/index.md PRE-CREATION 
  docs/endpoints/files/debug.md PRE-CREATION 
  docs/endpoints/files/download.json/index.md PRE-CREATION 
  docs/endpoints/files/download.md PRE-CREATION 
  docs/endpoints/files/read.json/index.md PRE-CREATION 
  docs/endpoints/files/read.md PRE-CREATION 
  docs/endpoints/index.md PRE-CREATION 
  docs/endpoints/logging/toggle.md PRE-CREATION 
  docs/endpoints/master/api/v1/scheduler.md PRE-CREATION 
  docs/endpoints/master/create-volumes.md PRE-CREATION 
  docs/endpoints/master/destroy-volumes.md PRE-CREATION 
  docs/endpoints/master/flags.md PRE-CREATION 
  docs/endpoints/master/frameworks.md PRE-CREATION 
  docs/endpoints/master/health.md PRE-CREATION 
  docs/endpoints/master/machine/down.md PRE-CREATION 
  docs/endpoints/master/machine/up.md PRE-CREATION 
  docs/endpoints/master/maintenance/schedule.md PRE-CREATION 
  docs/endpoints/master/maintenance/status.md PRE-CREATION 
  docs/endpoints/master/observe.md PRE-CREATION 
  docs/endpoints/master/quota.md PRE-CREATION 
  docs/endpoints/master/redirect.md PRE-CREATION 
  docs/endpoints/master/reserve.md PRE-CREATION 
  docs/endpoints/master/roles.json/index.md PRE-CREATION 
  docs/endpoints/master/roles.md PRE-CREATION 
  docs/endpoints/master/slaves.md PRE-CREATION 
  docs/endpoints/master/state-summary.md PRE-CREATION 
  docs/endpoints/master/state.json/index.md PRE-CREATION 
  docs/endpoints/master/state.md PRE-CREATION 
  docs/endpoints/master/tasks.json/index.md PRE-CREATION 
  docs/endpoints/master/tasks.md PRE-CREATION 
  docs/endpoints/master/teardown.md PRE-CREATION 
  docs/endpoints/master/unreserve.md PRE-CREATION 
  docs/endpoints/metrics/snapshot.md PRE-CREATION 
  docs/endpoints/monitor/statistics.json/index.md PRE-CREATION 
  docs/endpoints/monitor/statistics.md PRE-CREATION 
  docs/endpoints/profiler/start.md PRE-CREATION 
  docs/endpoints/profiler/stop.md PRE-CREATION 
  docs/endpoints/registrar/registry.md PRE-CREATION 
  docs/endpoints/slave/api/v1/executor.md PRE-CREATION 
  docs/endpoints/slave/flags.md PRE-CREATION 
  docs/endpoints/slave/health.md PRE-CREATION 
  docs/endpoints/slave/state.json/index.md PRE-CREATION 
  docs/endpoints/slave/state.md PRE-CREATION 
  docs/endpoints/system/stats.json/index.md PRE-CREATION 
  docs/endpoints/version.md PRE-CREATION 
  site/Rakefile 0ce4b7975f95ab6930f0b2674191930df9ab5b20 

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


Testing
---


Thanks,

Kevin Klues

Review Request 42788: Fixed whitespace errors in help strings for 3rdparty/libprocess.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

Previously some of the help strings includes a blank space at the end of
some of their lines of text. This was causing problems when using these
strings to generate .md files and then attempting to commit them back to
the repo.

This commit removes this unnecessary whitespace.


Diffs
-

  3rdparty/libprocess/src/metrics/metrics.cpp 
985fb5fcf3201ca767e6defc72a10b412ffd20a1 

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


Testing
---


Thanks,

Kevin Klues

Review Request 42787: Fixed whitespace errors in help strings for mesos/src.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

Previously some of the help strings included a blank space at the end of
some of their lines of text. This was causing problems when using these
strings to generate .md files and then attempting to commit them back to
the repo.

This commit removes this unnecessary whitespace.


Diffs
-

  src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
  src/master/registrar.cpp c46cc064413c02e22d48a47a17f8d3df2546c9c9 
  src/slave/http.cpp 0ba2e82fd8ce58508f364aff761d50dc4f264f65 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 42791: Added link to Http Endpoints doc in home.md.

[Kevin Klues]

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

(Updated Jan. 26, 2016, 7:49 a.m.)


Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Changes
---

Add links to test docs site in 'Testing' section.


Repository: mesos


Description
---

Added link to Http Endpoints doc in home.md.


Diffs
-

  docs/home.md dea6ec2605662dbd4b10d69b2bf8f35af50389ec 

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


Testing (updated)
---

Documentation is live here:
http://c99.millennium.berkeley.edu/documentation/latest/
http://c99.millennium.berkeley.edu/documentation/latest/endpoints


Thanks,

Kevin Klues

Re: Review Request 42783: Fixed error message style in 'roles::validate'.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 26, 2016, 5:39 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42783/
> ---
> 
> (Updated 一月 26, 2016, 5:39 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 
> 
> Diff: https://reviews.apache.org/r/42783/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Timothy Chen]

> On Jan. 25, 2016, 8:43 a.m., Bernd Mathiske wrote:
> > src/tests/health_check_tests.cpp, line 351
> > 
> >
> > Instead of naked strings we should reuse a constant that we can change 
> > in one place.

I think I'll punt on this for now.


- Timothy


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


On Jan. 25, 2016, 7:18 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 25, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7. Docker daemon is reporting too many routes/links after downloading 
> busybox, which is exactly the issue referred in the previous link.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 42784: Allocator Benchmark: Pull resource parsing out of for loop.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 26, 2016, 5:47 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42784/
> ---
> 
> (Updated 一月 26, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> b1cb955b7eb1213c7ba4a9c5181545bb49154f06 
> 
> Diff: https://reviews.apache.org/r/42784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 41772: Added helper function to flatten resources.

[Klaus Ma]

> On Jan. 20, 2016, 6:18 a.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > 
> >
> > It should be fine to just name this `flatten`.
> > 
> > You should also consider changing the parameter type from 
> > `RevocableInfo::Type` to just `RevocableInfo`.  Supplying the whole 
> > `RevocableInfo` will save us another tweak to the helpers if we change the 
> > revocable protobufs again.
> 
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not 
> know what does `flatten()` mean.
> 
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean 
> change `RevocableInfo` from `message` to `enum`? I think it's better to 
> define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. 
> `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = 
> Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
> 
> Regarding `flatten`, there're two options to me:
> 
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current 
> `flatten` with `None()` as default vaule, and a new helper function to accept 
> `RevocableInfo::Type` only without default value. The sample code is:
> 
> ```
> Resources flatten(string role = "*", Option 
> resveration = None(), Option revocable = None()) {
>   ...
>   if (revocable.isSome()) {
> flatten(revocable.get());
>   } else {
> // clear revocable info.
>   }
> }
> 
> Resources flatten(RevocableInfo::Type revocable) {
>   foreach resources {
> resource.mutable_revocable()->set_type(revocable);
>   }
> }
> ```
> 
> * Option 2: add new `flatten` functions but did not provide default 
> value; `None()` means clear the `RevocableInfo::Type`.
> 
> I perfer to Option 1 :). Any comments?
> 
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need 
> to input all of the four parameter, it is really not convenient for the 
> caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` 
> parameter, I think that maybe 2 is better.
> 
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and 
> revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call 
> `flatten(Resources::ALLOCATION_SLACK)`.
> 
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
> 
> Option 1) has involved two APIs, one is update current `Resources 
> flatten(string role = "*", Option resveration = None(), , 
> Option revocable = None() )`, the other is add a new 
> `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to 
> clear `allocation slack` and the `second flatten` is used to set `allocation 
> slack` flag.
> 
> Seems 1) is better as it faciliates the caller.
> 
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`  // No default so 
> there's no ambiguity.
> 
> ---
> 
> Note: If you wanted a single `flatten`, you'd need to have 
> `Option` so that you have the option of setting, 
> un-setting, or not-changing each field.
> 
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better 
> and clear.
> 
> Guangya Liu wrote:
> But the problem is that even with 
> 
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`
> 
> There are still ambiguity when calling `flatten(role)`, comments?
> 
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 
> 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
>~^
> What about still use `flattenSlack`?
> 
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make 
> check` failed, right?
> 
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?
> 
> Joseph Wu wrote:
> It's possible to fix the ambiguity (thanks MPark!), although I'm not sure 
> if we want to...
> 
> We can disambiguate by changing one of `Option` 's constructors to:
> ```
> template <
>   typename U,
>   typename = typename std::enable_if<
>   std::is_constructible::value>::type>
> Option(const U& u);
> ```
> 
> Guangya Liu wrote:
> Compared with this complex one, I prefer still use another name such as 
> `flattenRevocable` instead, cause we are now 

Review Request 42791: Added link to Http Endpoints doc in home.md.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

Added link to Http Endpoints doc in home.md.


Diffs
-

  docs/home.md dea6ec2605662dbd4b10d69b2bf8f35af50389ec 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 42594: Introduced framework registry operations.

[Yongqiao Wang]

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

(Updated Jan. 26, 2016, 7:54 a.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Update the code logic and format.


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


Repository: mesos


Description
---

Introduced framework registry operations.


Diffs (updated)
-

  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 

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


Testing
---

Make && Make check successfully!


Thanks,

Yongqiao Wang

Re: Review Request 42588: Added unit-test for `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Jan. 26, 2016, 6:32 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added unit-test for `NetClsHandleMgr`.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
6510553b412ae70f981949754c207fb5f7e1c220 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Re: Review Request 42783: Fixed error message style in 'roles::validate'.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42761, 42762, 42783]

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

- Mesos ReviewBot


On Jan. 26, 2016, 5:39 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42783/
> ---
> 
> (Updated Jan. 26, 2016, 5:39 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 
> 
> Diff: https://reviews.apache.org/r/42783/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42047: Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to isolate a mesos container using the net_cls cgroup subsystem.

[Cong Wang]

> On Jan. 15, 2016, 6 a.m., Cong Wang wrote:
> > Why do we need netcls to regulate framework traffic on a per-container 
> > basis? Given the fact that a) the port range based filters already work and 
> > the code (see egress fq_codel) already exists b) we only have port range 
> > based network isolation so far.
> > 
> > I see no point of this. Please describe your use case with details, just 
> > pointing to netcls kernel doc doesn't help at all.
> 
> Cong Wang wrote:
> Since no one answers this, I assume no one in Mesosphere actually 
> understands it... So looks like you are pushing something no one is actually 
> going to use.
> 
> Avinash sridharan wrote:
> The egress_fq_codel that you are pointing too (I am assuming this is the 
> jira you are refferring to https://issues.apache.org/jira/browse/MESOS-2422) 
> needs port mapping isolator to enforce QoS on any egress traffic 
> shaping/policing, and for that matter any network policy enforcement.  
> 
> The net_cls cgroup is a linux kernel construct that allows operators to 
> support traffic shapping/policing and any network policy enforcement using 
> existing networking tools like tc and iptables. By enabling net_cls cgroup it 
> gives mesos a more generalized way of allowing operators to enforce network 
> policy irrespective of whether the task is running in the global namespace or 
> in a specific network namespace. In other words it will allow network policy 
> enforcement to take place irrespective of the type of network isolator you 
> are using. For e.g., if someone wants to use ip-per-container (MESOS-2044) vs 
> the port mapping isolator, operators would still be able to perform policy 
> enforcement without relying on the network isolator to provide those 
> constructs.
> 
> Cong Wang wrote:
> True, I know what netcls is more than you do, but you just ignore the 
> fact that we _only_ have port mapping isolator in our _current_ code, that is 
> my whole point. We can always add this _after_ ip-per-container work is 
> merged in upstream, it is never too late.
> 
> No need to mention this is hard to work together with the fq_codel 
> filters on egress. This is why I ask for more details, but you still don't 
> give any detail so far.
> 
> Jie Yu wrote:
> Cong, we already have other network isolators (e.g., Calico has one for 
> ip per container) through modules. I don't think the operator will allow two 
> network isolators to co-exists in production so I am not so worried about the 
> egress filter conflicts.

I know Calico is ip-per-container, but my point is Calico module is 
out-of-tree, why do we need this in tree just for an out-of-tree module? IOW, 
why not just let Calico carry this code?

Also, you think the operator will not allow that, but you don't document it at 
all...


- Cong


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


On Jan. 20, 2016, 5:05 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42047/
> ---
> 
> (Updated Jan. 20, 2016, 5:05 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4262
> https://issues.apache.org/jira/browse/MESOS-4262
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Specified the CgroupsNetClsIsolatorProcess class. This adds the ability to 
> isolate a mesos container using the net_cls cgroup subsystem.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt a52203ab9aa47315e6e5c58cc283a7b5df597c76 
>   src/Makefile.am 4fabe600d4ba38c95a777d622b0b675dd5811a53 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42047/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 42490: Updated Presentaions.md and fixed two typos in support/hooks.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 19, 2016, 8:51 a.m., Disha  Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42490/
> ---
> 
> (Updated 一月 19, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Guangya Liu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Presentaions.md and fixed two typos in support/hooks.
> 
> 
> Diffs
> -
> 
>   docs/presentations.md 2ce4b12 
>   support/hooks/post-rewrite af907de 
>   support/hooks/pre-commit bdc12af 
> 
> Diff: https://reviews.apache.org/r/42490/diff/
> 
> 
> Testing
> ---
> 
> No testing required. Not a functional change.
> 
> 
> Thanks,
> 
> Disha  Singh
> 
>

Re: Review Request 42762: Improved performance of 'roles::validate'.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42761, 42762]

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

- Mesos ReviewBot


On Jan. 26, 2016, 12:10 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42762/
> ---
> 
> (Updated Jan. 26, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This validation is used inside resource math which in turn is used
> heavily in the code base.
> The temporary string allocations caused a significant performance
> degradation in this critical path.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 
> 
> Diff: https://reviews.apache.org/r/42762/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 41491: Exposed docker image manifest to mesos containerizer.

[Gilbert Song]

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

(Updated Jan. 25, 2016, 9 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Qian Zhang, and Timothy 
Chen.


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


Repository: mesos


Description
---

Exposed docker image manifest to mesos containerizer.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
03425daaf015cf231920b7eda1d5424895a41286 
  src/slave/containerizer/mesos/provisioner/provisioner.hpp 
4202f5552174ad3ab595ab3f16ab91d0854951f0 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
023f7363f33c4a927fae11037a408f6747fc3c39 
  src/slave/containerizer/mesos/provisioner/store.hpp 
aec725f789f7aeb92abfcc6718c2e6e2f1f37981 
  src/tests/containerizer/provisioner_docker_tests.cpp 
33df60065903228749833bbad20449ba8784594a 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 42761: Resource Math: Substituted DeleteSubrange with Swap + RemoveLast.

[Joris Van Remoortere]

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

(Updated Jan. 26, 2016, 5:38 a.m.)


Review request for mesos and Michael Park.


Changes
---

Added comment.
Updated v1.


Repository: mesos


Description
---

This improves 'operator-=' performance for Resources.


Diffs (updated)
-

  src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
  src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 26, 2016, 1:16 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42288/
> ---
> 
> (Updated 一月 26, 2016, 1:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4328
> https://issues.apache.org/jira/browse/MESOS-4328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker container REST API /monitor/statistics.json output have no timestamp 
> field, in docker.cpp function cgroupsStatistics add the timestamp value set.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp d2b77e3 
> 
> Diff: https://reviews.apache.org/r/42288/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 42586: Defined the NetClsHandleMgr class.

[Avinash sridharan]

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

(Updated Jan. 26, 2016, 6:11 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Defined the NetClsHandleMgr class.


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


Repository: mesos


Description
---

Defined the NetClsHandleMgr class. This class will be responsible for keeping 
track of the free and allocated net_cls handles.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
7ce5243ed7476a1cfd1371b5ce25b62dc07432db 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
0798de747627ccc45b01a2668e16693757dc69a8 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 42278: Fixed volume paths for command tasks with image.

[Timothy Chen]

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

(Updated Jan. 26, 2016, 6:37 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Jojy Varghese.


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


Repository: mesos


Description
---

Fixed volume paths for command tasks with image.


Diffs (updated)
-

  src/launcher/executor.cpp 356d311fdf97b2c4663c60e13ede7cdb71a264c7 
  src/slave/slave.cpp 1f4c8368feb0ce19963577582ce745acfb21aa9f 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
496275a73601664b51155ef1373d8d46b9069613 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 42762: Improved performance of 'roles::validate'.

[Joris Van Remoortere]

> On Jan. 26, 2016, 2:14 a.m., Anand Mazumdar wrote:
> > src/common/roles.cpp, line 62
> > 
> >
> > Not yours: Can we remove the period at the end of each of the error 
> > messages?
> > 
> > We dump them with the periods later: 
> > https://github.com/apache/mesos/blob/master/src/master/master.cpp#L635

Updated in the review after this one.
Thanks for catching this.


- Joris


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


On Jan. 26, 2016, 12:10 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42762/
> ---
> 
> (Updated Jan. 26, 2016, 12:10 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This validation is used inside resource math which in turn is used
> heavily in the code base.
> The temporary string allocations caused a significant performance
> degradation in this critical path.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 
> 
> Diff: https://reviews.apache.org/r/42762/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 42783: Fixed error message style in 'roles::validate'.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 42587: Implemented the `NetClsHandleMgr` class.

[Avinash sridharan]

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

(Updated Jan. 26, 2016, 6:31 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Implemented the `NetClsHandleMgr` class.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
7ce5243ed7476a1cfd1371b5ce25b62dc07432db 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
0798de747627ccc45b01a2668e16693757dc69a8 

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


Testing (updated)
---

make


Thanks,

Avinash sridharan

Re: Review Request 42784: Allocator Benchmark: Pull resource parsing out of for loop.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42761, 42762, 42783, 42784]

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

- Mesos ReviewBot


On Jan. 26, 2016, 5:47 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42784/
> ---
> 
> (Updated Jan. 26, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> b1cb955b7eb1213c7ba4a9c5181545bb49154f06 
> 
> Diff: https://reviews.apache.org/r/42784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42783: Fixed error message style in 'roles::validate'.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Jan. 26, 2016, 5:39 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42783/
> ---
> 
> (Updated Jan. 26, 2016, 5:39 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 5be807018ff84174cf0cabb933f6828d458d3846 
> 
> Diff: https://reviews.apache.org/r/42783/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42761: Resource Math: Substituted DeleteSubrange with Swap + RemoveLast.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On Jan. 26, 2016, 5:38 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42761/
> ---
> 
> (Updated Jan. 26, 2016, 5:38 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This improves 'operator-=' performance for Resources.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
>   src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 
> 
> Diff: https://reviews.apache.org/r/42761/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 42784: Allocator Benchmark: Pull resource parsing out of for loop.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Jan. 26, 2016, 5:47 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42784/
> ---
> 
> (Updated Jan. 26, 2016, 5:47 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> b1cb955b7eb1213c7ba4a9c5181545bb49154f06 
> 
> Diff: https://reviews.apache.org/r/42784/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 42786: Exposed the global 'help' process and added getter functions to it.

[Kevin Klues]

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

Review request for mesos, Ben Mahler, Greg Mann, and Neil Conway.


Repository: mesos


Description
---

Previously, there was no way to extract the help strings from the help
process once they had been installed into it.  The only way to get at
them was to visit the http endpoint they were associated with and pull
it from there.

This commit adds API calls to the process::Help class to get at these
strings more easily.


Diffs
-

  3rdparty/libprocess/include/process/help.hpp 
2e76a6a5b1069abce879374a88cea65036873f1d 
  3rdparty/libprocess/include/process/process.hpp 
1e4126f1082c24debd9d713b490d18f5ed83f0be 
  3rdparty/libprocess/src/help.cpp 2f718b9e160113518fb4a0260db916cb2242dbae 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 41772: Added helper function to flatten resources.

[Guangya Liu]

> On Jan. 19, 2016, 10:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > 
> >
> > It should be fine to just name this `flatten`.
> > 
> > You should also consider changing the parameter type from 
> > `RevocableInfo::Type` to just `RevocableInfo`.  Supplying the whole 
> > `RevocableInfo` will save us another tweak to the helpers if we change the 
> > revocable protobufs again.
> 
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not 
> know what does `flatten()` mean.
> 
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean 
> change `RevocableInfo` from `message` to `enum`? I think it's better to 
> define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. 
> `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = 
> Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
> 
> Regarding `flatten`, there're two options to me:
> 
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current 
> `flatten` with `None()` as default vaule, and a new helper function to accept 
> `RevocableInfo::Type` only without default value. The sample code is:
> 
> ```
> Resources flatten(string role = "*", Option 
> resveration = None(), Option revocable = None()) {
>   ...
>   if (revocable.isSome()) {
> flatten(revocable.get());
>   } else {
> // clear revocable info.
>   }
> }
> 
> Resources flatten(RevocableInfo::Type revocable) {
>   foreach resources {
> resource.mutable_revocable()->set_type(revocable);
>   }
> }
> ```
> 
> * Option 2: add new `flatten` functions but did not provide default 
> value; `None()` means clear the `RevocableInfo::Type`.
> 
> I perfer to Option 1 :). Any comments?
> 
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need 
> to input all of the four parameter, it is really not convenient for the 
> caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` 
> parameter, I think that maybe 2 is better.
> 
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and 
> revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call 
> `flatten(Resources::ALLOCATION_SLACK)`.
> 
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
> 
> Option 1) has involved two APIs, one is update current `Resources 
> flatten(string role = "*", Option resveration = None(), , 
> Option revocable = None() )`, the other is add a new 
> `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to 
> clear `allocation slack` and the `second flatten` is used to set `allocation 
> slack` flag.
> 
> Seems 1) is better as it faciliates the caller.
> 
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`  // No default so 
> there's no ambiguity.
> 
> ---
> 
> Note: If you wanted a single `flatten`, you'd need to have 
> `Option` so that you have the option of setting, 
> un-setting, or not-changing each field.
> 
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better 
> and clear.
> 
> Guangya Liu wrote:
> But the problem is that even with 
> 
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`
> 
> There are still ambiguity when calling `flatten(role)`, comments?
> 
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 
> 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
>~^
> What about still use `flattenSlack`?
> 
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make 
> check` failed, right?
> 
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?
> 
> Joseph Wu wrote:
> It's possible to fix the ambiguity (thanks MPark!), although I'm not sure 
> if we want to...
> 
> We can disambiguate by changing one of `Option` 's constructors to:
> ```
> template <
>   typename U,
>   typename = typename std::enable_if<
>   std::is_constructible::value>::type>
> Option(const U& u);
> ```

Compared with this complex one, I prefer still use another name such as 
`flattenRevocable` instead, cause we are now introducing another revocable 

Review Request 42780: Changed the NetClsIsolatorTest to check for net_cls handles.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Changed the NetClsIsolatorTest to check for net_cls handles.


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
6510553b412ae70f981949754c207fb5f7e1c220 

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


Testing
---


Thanks,

Avinash sridharan

Re: Review Request 42618: Modified the `cgroups/net_cls` isolator to use the `NetClsHandleMgr`.

[Avinash sridharan]

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

(Updated Jan. 26, 2016, 6:35 a.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Modified the `cgroups/net_cls` isolator to use the `NetClsHandleMgr`.


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


Repository: mesos


Description
---

Modified the `cgroups/net_cls` isolator to use the `NetClsHandleMgr` to manage 
net_cls handles.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
7ce5243ed7476a1cfd1371b5ce25b62dc07432db 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
0798de747627ccc45b01a2668e16693757dc69a8 

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


Testing (updated)
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 42751: Tweaked some resource test cases.

[Guangya Liu]

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




src/tests/resources_tests.cpp (line 1603)


Why do you update here?



src/tests/resources_tests.cpp (lines 1672 - 1673)


Is this useful? I think that we already covered the `same` contain case in 
`EXPECT_TRUE((r1 + r2).contains(r1 + r2));`


- Guangya Liu


On 一月 25, 2016, 10:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42751/
> ---
> 
> (Updated 一月 25, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We should check that two reservations with the same role but different
> principals are considered distinct.
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
> 
> Diff: https://reviews.apache.org/r/42751/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 42603: Added an http::Authenticator factory.

[Alexander Rojas]

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

(Updated Jan. 25, 2016, 1:44 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Jan Schlicht, 
and Till Toenshoff.


Changes
---

Open up review.


Summary (updated)
-

Added an http::Authenticator factory.


Repository: mesos


Description (updated)
---

Move the code required to instanciate instances of 
`process::http::authentication::Authenticator`
in `src/master/main.cpp` to its own factory.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
  src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
  src/authentication/http/authenticator.cpp PRE-CREATION 
  src/authentication/http/basic_authenticator_factory.cpp 
6eb1c5bd09b136d3bc20481ddcc65cb8bd153682 
  src/examples/test_http_authenticator_module.cpp 
acf51a6deb8e7dc4ab6ac0cf70380ddbb1839906 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/tests/http_authentication_tests.cpp 
bd622576973648e0dfeae1453a5ce631e4171352 

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


Testing (updated)
---

make check


Thanks,

Alexander Rojas

Re: Review Request 42247: Made sure the container launcher terminated before we leave the test.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 一月 25, 2016, 2:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42247/
> ---
> 
> (Updated 一月 25, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Till Toenshoff.
> 
> 
> Bugs: MESOS-4329
> https://issues.apache.org/jira/browse/MESOS-4329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Waiting for the launch result is not enough as it is unrelated to
> whether the launched container has terminated. If we do not wait for
> that the global teardown might find it still running and fail the tests
> (this can happen if we e.g., execute this test in isolation).
> 
> Also aline the name of the launch result to follow the canonical
> scheme.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp e943ab99baf3d74679a5da89a89f6a4b7ead 
> 
> Diff: https://reviews.apache.org/r/42247/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X 10.10.5)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 41597: Extending allocator interface to support dynamic weights.

[Yongqiao Wang]

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

(Updated Jan. 25, 2016, 3:06 p.m.)


Review request for mesos, Adam B, Neil Conway, and Qian Zhang.


Changes
---

Rebase to fix the build error for patch #41790.


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


Repository: mesos


Description (updated)
---

Extending allocator interface to support dynamic weights.


Diffs (updated)
-

  include/mesos/master/allocator.hpp e163669c9c4e4c98572968f18987704b60722a79 
  include/mesos/mesos.proto 96b911fb370223933df52f9370897871827d2247 
  include/mesos/v1/mesos.proto 0501dfa27ed610666226953591a902eac4c295f8 
  src/master/allocator/mesos/allocator.hpp 
581eaad376e7b2febe0b6359014617b935a677a3 
  src/master/allocator/mesos/hierarchical.hpp 
2d01034f43c3653f6233792ee6614fa311249e5c 
  src/master/allocator/mesos/hierarchical.cpp 
65c7e6b15c5308c0910667e1b12f39b21293a316 
  src/master/allocator/sorter/drf/sorter.hpp 
050896e8b12cd4097ccd137d5284d6b39b0f06ab 
  src/master/allocator/sorter/drf/sorter.cpp 
3a442f121f3a1505513877a5c78458a4b8d0a824 
  src/master/allocator/sorter/sorter.hpp 
7be6b44a762fd62c2cd7f28b4dc4865a4587ed26 
  src/tests/allocator.hpp 206e9ac3a83038a691f7929bdd627042b0f363b0 

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


Testing
---

Make & Make check successfully!

Test case: https://reviews.apache.org/r/41672/


Thanks,

Yongqiao Wang

Re: Review Request 42247: Made sure the container launcher terminated before we leave the test.

[Benjamin Bannier]

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

(Updated Jan. 25, 2016, 3:35 p.m.)


Review request for mesos, Jan Schlicht and Till Toenshoff.


Changes
---

Addressed comments and fix language issues in the commit_msg.


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


Repository: mesos


Description (updated)
---

Waiting for the launch result is not enough as it is unrelated to
whether the launched container has terminated. If we do not wait for
that the global teardown might find it still running and fail the tests
(this can happen if we e.g., execute this test in isolation).

Also aline the name of the launch result to follow the canonical
scheme.


Diffs (updated)
-

  src/tests/slave_tests.cpp e943ab99baf3d74679a5da89a89f6a4b7ead 

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


Testing
---

make check (OS X 10.10.5)


Thanks,

Benjamin Bannier

Re: Review Request 41681: Introduce HTTP endpoint /weights for updating weight.

[Yongqiao Wang]

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

(Updated Jan. 25, 2016, 3:23 p.m.)


Review request for mesos, Adam B, Neil Conway, and Qian Zhang.


Changes
---

Remove the key(weights) from JSON body to keep consistant with other endpoint 
(such as /quota).


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


Repository: mesos


Description
---

Introduce HTTP endpoint /weights for updating weight.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
5ee3c7afadd131802c93febbb6b4dbad069c2d81 
  include/mesos/authorizer/authorizer.proto 
226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
  src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
  src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
  src/authorizer/local/authorizer.hpp c7321c276d566eca6a91f45c546468bea1b0da15 
  src/authorizer/local/authorizer.cpp 9557bbdf68ff182c4538bbf70cee576d717abc05 
  src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
  src/master/weights_handler.cpp PRE-CREATION 
  src/tests/mesos.hpp 5a737a6490060b0194db097990b327c9921221f4 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

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


Testing
---

Make & Make check successfully!

$ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
--weights="role1=4.2,role2=3.1" --authenticate_http 
--credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [ ], 
"name": "*", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 1
}, 
{
"frameworks": [ ], 
"name": "role1", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 4.2
}, 
{
"frameworks": [ ], 
"name": "role2", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 3.1
}
]
}

Test update:
$ curl --user framework1:secret_string1 --data 
weights="[{\"weight\":1.8,\"role\":\"role1\"},{\"weight\":1.0,\"role\":\"role2\"},{\"weight\":3.4,\"role\":\"role3\"}]"
 -X PUT http://127.0.0.1:5050/weights
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [],
"name": "*",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role1",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.8
},
{
"frameworks": [],
"name": "role2",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role3",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 3.4
}
]
}

Test recovuery:
$ ps -ef | grep mesos-master
501 56292 1   0  6:18PM ttys0010:00.31 
/Users/yqwyq/Desktop/mesos/build/src/.libs/mesos-master --ip=127.0.0.1 
--work_dir=/tmp/mesos-master --weights=role1=4.2,role2=3.1 --authenticate_http 
--credentials=/opt/credentials.json
$ kill -9 56292

$ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
--weights="role1=4.2,role2=3.1,role6=9.0" --authenticate_http 
--credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [],
"name": "*",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role1",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.8
},
{
"frameworks": [],
"name": "role2",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
 

Re: Review Request 41789: Expose the http::internal::request function.

[Yongqiao Wang]

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

(Updated Jan. 25, 2016, 3:26 p.m.)


Review request for mesos, Adam B, Joerg Schad, Joris Van Remoortere, Neil 
Conway, and Qian Zhang.


Changes
---

Rebase to address merge error.


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


Repository: mesos


Description
---

Expose the http::internal::request function.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
bcba3040e902b34ef6707fe885595bcf59eb964b 
  3rdparty/libprocess/src/http.cpp 762da9a9038fc0a81156b5c03b556084df1bd7e0 
  3rdparty/libprocess/src/tests/http_tests.cpp 
66d185e3fd8a165d8a98d3d2752e756f8de3499b 

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


Testing
---

Make & Make check successfully.


Thanks,

Yongqiao Wang

Re: Review Request 42648: Moved http authenticator initialization to main.

[Alexander Rojas]

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

(Updated Jan. 25, 2016, 1:57 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Jan Schlicht, 
and Till Toenshoff.


Summary (updated)
-

Moved http authenticator initialization to main.


Repository: mesos


Description (updated)
---

Moves the code which initializes instances of 
`process::http::authentication::Authenticator` from `mesos::internal::Master` 
to the master main file.

The change is needed in order to show that libprocess http authentication is 
turned at the system process level and not at the libprocess process level.


Diffs (updated)
-

  src/master/main.cpp 0d5ac49121491152d8426cd764a1f1a0f37483ae 

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


Testing (updated)
---

make check

manual tests.


Thanks,

Alexander Rojas

Review Request 42719: Add doc for weights.

[Yongqiao Wang]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Add doc for weights.


Diffs
-

  docs/weights.md PRE-CREATION 

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


Testing
---


Thanks,

Yongqiao Wang

Re: Review Request 42458: Made links to .md files consistent across documentation.

[Joerg Schad]

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

(Updated Jan. 25, 2016, 1:50 p.m.)


Review request for mesos, Neil Conway and Till Toenshoff.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Made links to .md files consistent across documentation.


Diffs (updated)
-

  docs/allocation-module.md 1dc1abaf9e28abc01de728634593b42c7cfc8af5 
  docs/app-framework-development-guide.md 
e094f3242682863e05d3f6ee366b081cdda6645f 
  docs/authentication.md 5f13181e93df4dc15a62d5540eeb43c40f91344d 
  docs/c++-style-guide.md c045a8d5bef171fb475a7d6bc3e78ffb494a6d2e 
  docs/clang-format.md 206bc2ccf42e5593f715c6ccf0cc42e5cbc62469 
  docs/committers.md c1ae2574cef66c53886cb36e8ec38d474e3b19f4 
  docs/containerizer-internals.md 14c4c6ce5ed2a28defb7d6623c45ee337f1e6c3c 
  docs/containerizer.md a694de988f3326ac62ff52deba82b659a7080d8d 
  docs/deploy-scripts.md 0dce7d88cdcdeaf18b585b9c000faf25a1bd5dfa 
  docs/documentation-guide.md 9548bcabe95a2a26f222022530e6b66d3537c07b 
  docs/effective-code-reviewing.md fdfafc7d11f8455b68395cd2a4df6dab75f07b22 
  docs/fetcher-cache-internals.md b71d22591b8213be2e2a37555895176b8a6a4e19 
  docs/fetcher.md 3cf8875c03fe75daff70f482282d2860ac404734 
  docs/high-availability.md 02ac7c76d8a597e8457d4ea716e5915542fd2c8b 
  docs/home.md fa9c19c6d511b8acb7f27c301b21eb369c8f4b47 
  docs/operational-guide.md 811cfc4a634e44a1d2e8db40b44f6b744a1643dd 
  docs/sandbox.md a2ad226b0ee7969622495f090579f15029a2a083 
  docs/tools.md 3e5f4d786ccfcef85b341cdb698c4d0579019980 
  docs/versioning.md 7af6581f2eef10c78c76c07c44e88c6841465398 

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


Testing
---

Viewed via Docker Website Container (including changes to rakefile provided by 
previous review).


Thanks,

Joerg Schad

Re: Review Request 42016: Windows: Apply patch.exe without elevation prompt

[Alex Clemmer]

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




3rdparty/CMakeLists.txt (line 45)


Could we please delete the extra comment line here?



3rdparty/CMakeLists.txt (lines 45 - 70)


Similarly to my reasoning above, it seems like this should actually all go 
in `Mesos3rdpartyConfigure.cmake`.

EDIT: Oh, sorry, everything except the last line, where we generate the 
`ZOOKEEPER_PATCH_CMD`. That can stay in this file.



3rdparty/CMakeLists.txt (lines 46 - 47)


Could we please wrap these lines at 80 columns and put ``` characters 
aroudn the path?



3rdparty/CMakeLists.txt (line 48)


The comment here seems like it could be clearer -- it's not actually 
setting a directory, it's setting the environment variable that we use to 
retrieve the tmp directory, right?

I recommend just changing the code to something like the following (NOTE: I 
have NOT tested this):

```
  # Points at user temp directory, e.g. `\Users\\AppData\Local\Temp`.
  set(USER_TMP_DIR $ENV{"TMP"})
```

This would simplify some of the logic below also, as well as making the 
comment less confusing.



3rdparty/CMakeLists.txt (line 49)


Can we put a space between these "logical blocks" of code, please? Same 
goes for line 53.



3rdparty/CMakeLists.txt (line 61)


Can we please actually delete all the lines that are just empty comments?



3rdparty/CMakeLists.txt (line 63)


Can we please wrap this at 80 characters?

Also: can you please expand this comment slightly? For my own 
understanding, it is worth mentioning that `patch` does not require admin, but 
asks for it anyway, and this manifest just tells Windows to run it without. If 
you have a URL that shows this has precedent out in the community, that is even 
better.



3rdparty/CMakeLists.txt (line 66)


Confusingly, our CMake style differs a bit from our C++ style. Can we 
please make it like this:

```
  add_custom_command(
OUTPUT  patch.exe
COMMAND ${APPLY_PATCH_MANIFEST_COMMAND})
```

Note also that I've taken out the extra spaces between `COMMAND` and the 
variable. Please do this as well. :)



3rdparty/CMakeLists.txt (line 69)


Can we please add a space between the `#` and `Third`?



3rdparty/CMakeLists.txt (line 72)


Can we please delete the extra line here?



CMakeLists.txt (lines 88 - 116)


I believe this should go in `3rdparty/cmake/Mesos3rdPartyConfigure.cmake`. 
We like to have the `*Configure.cmake` files contain all the intense logic of 
configuring things like how to run the patch command, so that the 
`CMakeLists.txt` only do very simple things, like run the patch command 
(instead of configuring it and running it).

We do this for two reasons. First, because CMake has incredibly strange 
dynamic scoping rules for variable evaluation, which means that it is really 
worth collecting all the intense platform-dependent config logic into one 
place. If you don't, then it gets _really, really hard_ to tell when you're 
setting one variable, or why some other variable has the value it does.

Second, because when we have all the configuration logic in one place, it 
makes it much easier to reason about where configuration logic rests. For 
example, if I was looking for where we're configuring the `patch.exe` on 
Windows, I'd start by looking in `Mesos3rdpartyConfigure.cmake`, because I know 
we are patching things in the `3rdparty` directory (as well as 
`3rdparty/libprocess/3rdparty`) and so the most general configuration file it 
would be in is the one for the `3rdparty` directory.



CMakeLists.txt (lines 88 - 89)


Please make comments end with periods. Since this happens throughout the 
patch, I'll just mention it once instead of opening a billion issues for you to 
close. :)

Also this might be reworded a bit to make it clearer. Perhaps: `Configure 
Windows use of the GNU patch utility; we attempt to find it in its default 
location, but this path may be customized also.`



CMakeLists.txt (lines 95 - 96)


Looks like these two lines are > 80 characters long. Can we 

Review Request 42735: Make commit-msg hook portable.

[David Forsythe]

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

Review request for mesos, Artem Harutyunyan and Ian Downes.


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


Repository: mesos


Description
---

Make commit-msg hook portable.


Diffs
-

  support/hooks/commit-msg d173dfdf380b08a9b349589dfd33c53b3cdccc60 

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


Testing
---


Thanks,

David Forsythe

Re: Review Request 41772: Added helper function to flatten resources.

[Joseph Wu]

> On Jan. 19, 2016, 2:18 p.m., Joseph Wu wrote:
> > src/common/resources.cpp, line 880
> > 
> >
> > It should be fine to just name this `flatten`.
> > 
> > You should also consider changing the parameter type from 
> > `RevocableInfo::Type` to just `RevocableInfo`.  Supplying the whole 
> > `RevocableInfo` will save us another tweak to the helpers if we change the 
> > revocable protobufs again.
> 
> Guangya Liu wrote:
> I think that we cannot rename it to `flatten` as the compiler will not 
> know what does `flatten()` mean.
> 
> Klaus Ma wrote:
> Regarding from `RevocableInfo::Type` to `RevocableInfo`, do you mean 
> change `RevocableInfo` from `message` to `enum`? I think it's better to 
> define const in `Resources` instead of changing `RevocableInfo`'s type, e.g. 
> `static const Resource::RevocableInfo::Type Resources::ALLOCATION_SLACK = 
> Resource::RevocableInfo::Type::ALLOCATION_SLACK`.
> 
> Regarding `flatten`, there're two options to me:
> 
> * Option 1: add `RevocableInfo::Type` to as 3rd parameter in current 
> `flatten` with `None()` as default vaule, and a new helper function to accept 
> `RevocableInfo::Type` only without default value. The sample code is:
> 
> ```
> Resources flatten(string role = "*", Option 
> resveration = None(), Option revocable = None()) {
>   ...
>   if (revocable.isSome()) {
> flatten(revocable.get());
>   } else {
> // clear revocable info.
>   }
> }
> 
> Resources flatten(RevocableInfo::Type revocable) {
>   foreach resources {
> resource.mutable_revocable()->set_type(revocable);
>   }
> }
> ```
> 
> * Option 2: add new `flatten` functions but did not provide default 
> value; `None()` means clear the `RevocableInfo::Type`.
> 
> I perfer to Option 1 :). Any comments?
> 
> Guangya Liu wrote:
> option 1 will cause all caller who want to flatten allocation slack need 
> to input all of the four parameter, it is really not convenient for the 
> caller.
> Option 2 need to add a new `flatten` with a `requested` but not `option` 
> parameter, I think that maybe 2 is better.
> 
> Klaus Ma wrote:
> regarding 1, if want to clear the flags (role, reservation and 
> revocable), call `flatten()`; if want to set `ALLOCATION_SLACK`, call 
> `flatten(Resources::ALLOCATION_SLACK)`.
> 
> Guangya Liu wrote:
> Just clarify 1) here, I have some mis-understanding for it.
> 
> Option 1) has involved two APIs, one is update current `Resources 
> flatten(string role = "*", Option resveration = None(), , 
> Option revocable = None() )`, the other is add a new 
> `flatten(RevocableInfo::Type revocable)`. The `first flatten` is used to 
> clear `allocation slack` and the `second flatten` is used to set `allocation 
> slack` flag.
> 
> Seems 1) is better as it faciliates the caller.
> 
> Joseph Wu wrote:
> Given how you're using `flatten`, it would be better to have two calls:
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`  // No default so 
> there's no ambiguity.
> 
> ---
> 
> Note: If you wanted a single `flatten`, you'd need to have 
> `Option` so that you have the option of setting, 
> un-setting, or not-changing each field.
> 
> Guangya Liu wrote:
> I think that adding a new `flatten` without default value might be better 
> and clear.
> 
> Guangya Liu wrote:
> But the problem is that even with 
> 
> `Resources flatten(string role = "*", Option resveration 
> = None())`
> and 
> `Resources flatten(Option revocable)`
> 
> There are still ambiguity when calling `flatten(role)`, comments?
> 
> Guangya Liu wrote:
> In file included from ../../src/tests/fault_tolerance_tests.cpp:56:
> ../../src/tests/mesos.hpp:665:39: error: call to member function 
> 'flatten' is ambiguous
> remaining.find(TASK_RESOURCES.flatten(role));
>~^
> What about still use `flattenSlack`?
> 
> Klaus Ma wrote:
> Sorry, just checked this error; it seems we `make` passed but `make 
> check` failed, right?
> 
> Guangya Liu wrote:
> Yes, the `make tests/check` failed, any idea for this?

It's possible to fix the ambiguity (thanks MPark!), although I'm not sure if we 
want to...

We can disambiguate by changing one of `Option` 's constructors to:
```
template <
  typename U,
  typename = typename std::enable_if<
  std::is_constructible::value>::type>
Option(const U& u);
```


- Joseph


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

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Jan. 25, 2016, 7:18 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 25, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7. Docker daemon is reporting too many routes/links after downloading 
> busybox, which is exactly the issue referred in the previous link.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 42728: Changed code to add text description when Http call returns Forbidden.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On Jan. 25, 2016, 6:47 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42728/
> ---
> 
> (Updated Jan. 25, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-4448
> https://issues.apache.org/jira/browse/MESOS-4448
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed code to add text description when Http call returns Forbidden.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
>   src/master/quota_handler.cpp aa06cbc83d2c467c2422f1d27bb9507998953c44 
> 
> Diff: https://reviews.apache.org/r/42728/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 41790: Add tests for /weights endpoint.

[Yongqiao Wang]

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

(Updated Jan. 25, 2016, 3:47 p.m.)


Review request for mesos, Adam B, Neil Conway, and Qian Zhang.


Changes
---

Update the tests and trigger the ReviewBot again.


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


Repository: mesos


Description
---

Add tests for /weights endpoint.


Diffs (updated)
-

  src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
  src/tests/dynamic_weights_tests.cpp PRE-CREATION 

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


Testing
---

Make and Make check successfully!

./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
[==] Running 11 tests from 1 test case.
[--] Global test environment set-up.
[--] 11 tests from DynamicWeightsTest
[ RUN  ] DynamicWeightsTest.PutInvalidRequest
[   OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
[ RUN  ] DynamicWeightsTest.ZeroWeight
[   OK ] DynamicWeightsTest.ZeroWeight (38 ms)
[ RUN  ] DynamicWeightsTest.NegativeWeight
[   OK ] DynamicWeightsTest.NegativeWeight (38 ms)
[ RUN  ] DynamicWeightsTest.MissingRole
[   OK ] DynamicWeightsTest.MissingRole (43 ms)
[ RUN  ] DynamicWeightsTest.UnknownRole
[   OK ] DynamicWeightsTest.UnknownRole (36 ms)
[ RUN  ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
[   OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
[ RUN  ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
[   OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
[ RUN  ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
[   OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
[ RUN  ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
[   OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
[ RUN  ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
[   OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal 
(37 ms)
[ RUN  ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
[   OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
[--] 11 tests from DynamicWeightsTest (492 ms total)

[--] Global test environment tear-down
[==] 11 tests from 1 test case ran. (503 ms total)
[  PASSED  ] 11 tests.


Thanks,

Yongqiao Wang

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jie Yu]

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



I am wondering if you do see performance issue regarding this? We typically 
don't optimize the code too much until it becomes a problem. Also, we try to 
avoid global variable dependencies (i.e., static bool isCreateError in this 
case).

- Jie Yu


On Jan. 13, 2016, 11:08 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 13, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42719: Add doc for weights.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42719]

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

- Mesos ReviewBot


On Jan. 25, 2016, 3 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42719/
> ---
> 
> (Updated Jan. 25, 2016, 3 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-3945
> https://issues.apache.org/jira/browse/MESOS-3945
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add doc for weights.
> 
> 
> Diffs
> -
> 
>   docs/weights.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42719/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41681: Introduce HTTP endpoint /weights for updating weight.

[Yongqiao Wang]

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

(Updated Jan. 25, 2016, 3:36 p.m.)


Review request for mesos, Adam B, Neil Conway, and Qian Zhang.


Changes
---

Update JOSN body to remove the key(weights).


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


Repository: mesos


Description
---

Introduce HTTP endpoint /weights for updating weight.


Diffs
-

  include/mesos/authorizer/authorizer.hpp 
5ee3c7afadd131802c93febbb6b4dbad069c2d81 
  include/mesos/authorizer/authorizer.proto 
226441f8cbd6d0828bf1636cc08c21ffcc75e6a7 
  src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
  src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
  src/authorizer/local/authorizer.hpp c7321c276d566eca6a91f45c546468bea1b0da15 
  src/authorizer/local/authorizer.cpp 9557bbdf68ff182c4538bbf70cee576d717abc05 
  src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
  src/master/master.hpp 3a7e18232323a1c051bcc97915484b1195fffe58 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/master/registry.proto 9958f9c2bdb785390fca2f292b65d5a9310434d5 
  src/master/weights_handler.cpp PRE-CREATION 
  src/tests/mesos.hpp 5a737a6490060b0194db097990b327c9921221f4 
  src/tests/mesos.cpp 18d0d8f8037ebc27c87bcb0f1ce9f143e7505ec8 

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


Testing (updated)
---

Make & Make check successfully!

$ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
--weights="role1=4.2,role2=3.1" --authenticate_http 
--credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [ ], 
"name": "*", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 1
}, 
{
"frameworks": [ ], 
"name": "role1", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 4.2
}, 
{
"frameworks": [ ], 
"name": "role2", 
"resources": {
"cpus": 0, 
"disk": 0, 
"mem": 0
}, 
"weight": 3.1
}
]
}

Test update:
$ curl --user framework1:secret_string1 --data 
"[{\"weight\":1.8,\"role\":\"role1\"},{\"weight\":1.0,\"role\":\"role2\"},{\"weight\":3.4,\"role\":\"role3\"}]"
 -X PUT http://127.0.0.1:5050/weights
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [],
"name": "*",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role1",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.8
},
{
"frameworks": [],
"name": "role2",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role3",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 3.4
}
]
}

Test recovuery:
$ ps -ef | grep mesos-master
501 56292 1   0  6:18PM ttys0010:00.31 
/Users/yqwyq/Desktop/mesos/build/src/.libs/mesos-master --ip=127.0.0.1 
--work_dir=/tmp/mesos-master --weights=role1=4.2,role2=3.1 --authenticate_http 
--credentials=/opt/credentials.json
$ kill -9 56292

$ (./mesos-master.sh --ip=127.0.0.1 --work_dir=/tmp/mesos-master  
--weights="role1=4.2,role2=3.1,role6=9.0" --authenticate_http 
--credentials=/opt/credentials.json  >> /tmp/mesos-master.log 2>&1 &)
$ curl http://localhost:5050/roles | python -mjson.tool
{
"roles": [
{
"frameworks": [],
"name": "*",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
"frameworks": [],
"name": "role1",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.8
},
{
"frameworks": [],
"name": "role2",
"resources": {
"cpus": 0,
"disk": 0,
"mem": 0
},
"weight": 1.0
},
{
   

Re: Review Request 42247: Made sure the container launcher terminated before we leave the test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42247]

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

- Mesos ReviewBot


On Jan. 25, 2016, 2:35 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42247/
> ---
> 
> (Updated Jan. 25, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Till Toenshoff.
> 
> 
> Bugs: MESOS-4329
> https://issues.apache.org/jira/browse/MESOS-4329
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Waiting for the launch result is not enough as it is unrelated to
> whether the launched container has terminated. If we do not wait for
> that the global teardown might find it still running and fail the tests
> (this can happen if we e.g., execute this test in isolation).
> 
> Also aline the name of the launch result to follow the canonical
> scheme.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp e943ab99baf3d74679a5da89a89f6a4b7ead 
> 
> Diff: https://reviews.apache.org/r/42247/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X 10.10.5)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 42690: Moved ExecutorInfo and Option TaskInfo into ContainerConfig.

[Gilbert Song]

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

(Updated Jan. 25, 2016, 9:20 a.m.)


Review request for mesos, Jie Yu and Timothy Chen.


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


Repository: mesos


Description
---

Moved ExecutorInfo and Option TaskInfo into ContainerConfig.


Diffs
-

  include/mesos/slave/isolator.hpp c273d668cee1f8000d02247a04cf71573a407815 
  include/mesos/slave/isolator.proto c4b7b1f80f52b85632133e3423a89f92b0ff32f3 
  src/slave/containerizer/mesos/containerizer.hpp 
a8177fb4e9f503ddabf834551cb987a4f58d8961 
  src/slave/containerizer/mesos/containerizer.cpp 
075b3abbe100c72da4c78c0be33ba37473fc4a23 
  src/slave/containerizer/mesos/isolator.hpp 
8bdccde81d5a16506ccead917decda6f39a0abb1 
  src/slave/containerizer/mesos/isolator.cpp 
4de981047cd1b995d23e0e20af841035ca689ac6 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
9c331ebcdf1a12f8587b789c96d2eb843e6894b1 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
afab0b4b7a4ab50bf68efc6938735970a59192de 
  src/slave/containerizer/mesos/isolators/cgroups/mem.hpp 
944544da762371692eadcac18bfb7334a8a5bffa 
  src/slave/containerizer/mesos/isolators/cgroups/mem.cpp 
bc76a0b6940f6a17f820a24ec6b2c9d8dd449c4a 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.hpp 
7ce5243ed7476a1cfd1371b5ce25b62dc07432db 
  src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp 
0798de747627ccc45b01a2668e16693757dc69a8 
  src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
a996b102e3cf725121a024e2eaecc0f5e1d40232 
  src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
573621479b1d18c18a78b59d2c8c5a396595e50a 
  src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
3acbd6904de254bd5fd405ee2cd1f33b610dfa97 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
96c37ed7ac27074eb79e3eb5e0301b642ab653f2 
  src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
b36a196fa8e953143ec96aae7b39cba6ee1b01fa 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
d914587eedc9c66bc14b4088ec211b7f0eea63ea 
  src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
c3ac4488c5b1f3abe5bf59e5c9df80993c0f467d 
  src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
c1b6e086a85ca03acebc4844a6753ebb6cb58fc5 
  src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
e20b5841e355656c9be688d28af8b758fdae0b27 
  src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
fb21e6c6e9705118a94ee52fe66d74ee7b184376 
  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
ea58c42986ab2e795799bd9d193987450b766472 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
c8e5281f05a5f8c8c00adcc7b041053a7c10b5d5 
  src/slave/containerizer/mesos/isolators/posix.hpp 
7cdf9ed2678e098d9c83017b8ed8ec490dc2e964 
  src/slave/containerizer/mesos/isolators/posix/disk.hpp 
e718a111e6c5d1120ef767a16f286162d925ba64 
  src/slave/containerizer/mesos/isolators/posix/disk.cpp 
5d4381087677dae58b1fb6da0f47093a4a749d73 
  src/tests/containerizer/isolator.hpp 0f9b21aeab0a3d0facc803463b08a3259f569b8b 
  src/tests/containerizer/isolator_tests.cpp 
6510553b412ae70f981949754c207fb5f7e1c220 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
677fcc1c654f83ad3e60e0f6172a1a1e4a1045b1 
  src/tests/containerizer/port_mapping_tests.cpp 
fab16f697f90abc11d681222f10f518d70da908b 

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


Testing
---

make check (ubuntu14.04 + clang-3.6)


Thanks,

Gilbert Song

Re: Review Request 42016: Windows: Apply patch.exe without elevation prompt

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On Jan. 14, 2016, 12:43 a.m., M Lawindi wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42016/
> ---
> 
> (Updated Jan. 14, 2016, 12:43 a.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Alex Clemmer, M 
> Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows:[2/2] Added zookeeper-3.4.5 to Mesos build
> 
> 
> Diffs
> -
> 
>   3rdparty/CMakeLists.txt ac5c25a8797a687e84384682975ab99fb3e30448 
>   3rdparty/cmake/Mesos3rdpartyConfigure.cmake 
> 34e61ff90eca0ffdddb6b6b8e2f8e552691637fa 
>   3rdparty/patch.exe.manifest PRE-CREATION 
>   CMakeLists.txt 9b7044b7860fd64b854ac27b28a48d297dfdeae8 
>   src/slave/cmake/SlaveConfigure.cmake 
> cf378a27297474b2a9f338e0c832612370f7302a 
> 
> Diff: https://reviews.apache.org/r/42016/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> M Lawindi
> 
>

Re: Review Request 41790: Add tests for /weights endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [41597, 41681, 41789, 41790]

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

- Mesos ReviewBot


On Jan. 25, 2016, 3:47 p.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41790/
> ---
> 
> (Updated Jan. 25, 2016, 3:47 p.m.)
> 
> 
> Review request for mesos, Adam B, Neil Conway, and Qian Zhang.
> 
> 
> Bugs: MESOS-4200
> https://issues.apache.org/jira/browse/MESOS-4200
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for /weights endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8657a869f931aa7482fbb09f2c6df95b6a8c50c6 
>   src/tests/dynamic_weights_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41790/diff/
> 
> 
> Testing
> ---
> 
> Make and Make check successfully!
> 
> ./src/mesos-tests --gtest_filter=DynamicWeightsTest.*
> [==] Running 11 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 11 tests from DynamicWeightsTest
> [ RUN  ] DynamicWeightsTest.PutInvalidRequest
> [   OK ] DynamicWeightsTest.PutInvalidRequest (102 ms)
> [ RUN  ] DynamicWeightsTest.ZeroWeight
> [   OK ] DynamicWeightsTest.ZeroWeight (38 ms)
> [ RUN  ] DynamicWeightsTest.NegativeWeight
> [   OK ] DynamicWeightsTest.NegativeWeight (38 ms)
> [ RUN  ] DynamicWeightsTest.MissingRole
> [   OK ] DynamicWeightsTest.MissingRole (43 ms)
> [ RUN  ] DynamicWeightsTest.UnknownRole
> [   OK ] DynamicWeightsTest.UnknownRole (36 ms)
> [ RUN  ] DynamicWeightsTest.UpdateWeightsWithExplictRoles
> [   OK ] DynamicWeightsTest.UpdateWeightsWithExplictRoles (44 ms)
> [ RUN  ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles
> [   OK ] DynamicWeightsTest.UpdateWeightsWithImplicitRoles (52 ms)
> [ RUN  ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest
> [   OK ] DynamicWeightsTest.UnauthenticatedUpdateWeightRequest (40 ms)
> [ RUN  ] DynamicWeightsTest.AuthorizedWeightUpdateRequest
> [   OK ] DynamicWeightsTest.AuthorizedWeightUpdateRequest (32 ms)
> [ RUN  ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal
> [   OK ] DynamicWeightsTest.AuthorizedUpdateWeightRequestWithoutPrincipal 
> (37 ms)
> [ RUN  ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest
> [   OK ] DynamicWeightsTest.UnauthorizedWeightUpdateRequest (30 ms)
> [--] 11 tests from DynamicWeightsTest (492 ms total)
> 
> [--] Global test environment tear-down
> [==] 11 tests from 1 test case ran. (503 ms total)
> [  PASSED  ] 11 tests.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jojy Varghese]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).

Jie, thanks for looking at this. 

This change is two parts - one is simply replacing the initialization of 
`creators` hashmap. Initialization using universal `{` initialization is the 
preferred way in c++11 that calling `put`. 

The second change is to optimize the creation. I thought this was necessary 
because we will be calling `create` for every URI fetch in a image pull and its 
dependencies(or layers). As the information to create the fetcher does not 
change for each of that `create` call, I thought it would be better to reuse 
the initialized structures. 

We can remove the 2nd part (optimization) if you like.


- Jojy


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


On Jan. 13, 2016, 11:08 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 13, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 42728: Changed code to add text description when Http call returns Forbidden.

[Srinivas Brahmaroutu]

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

Review request for mesos.


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


Repository: mesos


Description
---

Changed code to add text description when Http call returns Forbidden.


Diffs
-

  src/master/http.cpp 12c1fe5a514903f657911302e8770e9b245fdbb7 
  src/master/quota_handler.cpp aa06cbc83d2c467c2422f1d27bb9507998953c44 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 42288: Add timestamp to DockerContainerizer's ResourceStatistics.

[Timothy Chen]

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


Fix it, then Ship it!




Ship It!


src/slave/containerizer/docker.cpp (line 1370)


I don't think this comment means much here. Let's remove it


- Timothy Chen


On Jan. 21, 2016, 6:18 a.m., Andy Pang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42288/
> ---
> 
> (Updated Jan. 21, 2016, 6:18 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-4328
> https://issues.apache.org/jira/browse/MESOS-4328
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Docker container REST API /monitor/statistics.json output have no timestamp 
> field, in docker.cpp function cgroupsStatistics add the timestamp value set.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 40f6f0b 
> 
> Diff: https://reviews.apache.org/r/42288/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Andy Pang
> 
>

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jie Yu]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).
> 
> Jojy Varghese wrote:
> Jie, thanks for looking at this. 
> 
> This change is two parts - one is simply replacing the initialization of 
> `creators` hashmap. Initialization using universal `{` initialization is the 
> preferred way in c++11 that calling `put`. 
> 
> The second change is to optimize the creation. I thought this was 
> necessary because we will be calling `create` for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that `create` call, I thought it would be 
> better to reuse the initialized structures. 
> 
> We can remove the 2nd part (optimization) if you like.
> 
> Jojy Varghese wrote:
> I could make the static flag `isCreateError` as atomic_flag if you prefer.

> The second change is to optimize the creation. I thought this was necessary 
> because we will be calling create for every URI fetch in a image pull and its 
> dependencies(or layers). As the information to create the fetcher does not 
> change for each of that create call, I thought it would be better to reuse 
> the initialized structures. 

Why we will be calling create for every URI fetch in a image pull? Can we save 
the uri::Fetcher instance? Eventually, there'll be only one uri::Fetcher at the 
top level (similar to the Fetcher right now).


- Jie


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


On Jan. 25, 2016, 9:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 25, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42735: Make commit-msg hook portable.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42735]

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

- Mesos ReviewBot


On Jan. 25, 2016, 8:07 p.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42735/
> ---
> 
> (Updated Jan. 25, 2016, 8:07 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Ian Downes.
> 
> 
> Bugs: MESOS-4502
> https://issues.apache.org/jira/browse/MESOS-4502
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make commit-msg hook portable.
> 
> 
> Diffs
> -
> 
>   support/hooks/commit-msg d173dfdf380b08a9b349589dfd33c53b3cdccc60 
> 
> Diff: https://reviews.apache.org/r/42735/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jie Yu]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).
> 
> Jojy Varghese wrote:
> Jie, thanks for looking at this. 
> 
> This change is two parts - one is simply replacing the initialization of 
> `creators` hashmap. Initialization using universal `{` initialization is the 
> preferred way in c++11 that calling `put`. 
> 
> The second change is to optimize the creation. I thought this was 
> necessary because we will be calling `create` for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that `create` call, I thought it would be 
> better to reuse the initialized structures. 
> 
> We can remove the 2nd part (optimization) if you like.
> 
> Jojy Varghese wrote:
> I could make the static flag `isCreateError` as atomic_flag if you prefer.
> 
> Jie Yu wrote:
> > The second change is to optimize the creation. I thought this was 
> necessary because we will be calling create for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that create call, I thought it would be 
> better to reuse the initialized structures. 
> 
> Why we will be calling create for every URI fetch in a image pull? Can we 
> save the uri::Fetcher instance? Eventually, there'll be only one uri::Fetcher 
> at the top level (similar to the Fetcher right now).
> 
> Jojy Varghese wrote:
> One reason I can think of is to allow operaters to install missing 
> plugings (say hdfs) without having to restart. Let me know if that use case 
> is invalid.

Even though, i would rather add interfaces to allow us to update an existing 
Fetcher, instead of creating a Fetcher each time we pull an image.


- Jie


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


On Jan. 25, 2016, 9:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 25, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Isabel Jimenez]

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


Ship it!




Ship It!

- Isabel Jimenez


On Jan. 25, 2016, 7:18 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 25, 2016, 7:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7. Docker daemon is reporting too many routes/links after downloading 
> busybox, which is exactly the issue referred in the previous link.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jojy Varghese]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).
> 
> Jojy Varghese wrote:
> Jie, thanks for looking at this. 
> 
> This change is two parts - one is simply replacing the initialization of 
> `creators` hashmap. Initialization using universal `{` initialization is the 
> preferred way in c++11 that calling `put`. 
> 
> The second change is to optimize the creation. I thought this was 
> necessary because we will be calling `create` for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that `create` call, I thought it would be 
> better to reuse the initialized structures. 
> 
> We can remove the 2nd part (optimization) if you like.

I could make the static flag `isCreateError` as atomic_flag if you prefer.


- Jojy


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


On Jan. 13, 2016, 11:08 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 13, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Neil Conway]

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




src/common/resources.cpp (line 66)


This is actually correct as written, no? i.e., if principal isn't set, 
`principal()` will return the empty string. Since an empty string isn't a legal 
principle, the comparison should just work, I believe.


- Neil Conway


On Jan. 25, 2016, 9:02 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Jan. 25, 2016, 9:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. During 
> the deprecation cycle, requests that do not have the field set are 
> invalidated and rejected. However, there remain a couple places in the code 
> that assume this field is always set. This patch uses `has_principal()` 
> before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jojy Varghese]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).
> 
> Jojy Varghese wrote:
> Jie, thanks for looking at this. 
> 
> This change is two parts - one is simply replacing the initialization of 
> `creators` hashmap. Initialization using universal `{` initialization is the 
> preferred way in c++11 that calling `put`. 
> 
> The second change is to optimize the creation. I thought this was 
> necessary because we will be calling `create` for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that `create` call, I thought it would be 
> better to reuse the initialized structures. 
> 
> We can remove the 2nd part (optimization) if you like.
> 
> Jojy Varghese wrote:
> I could make the static flag `isCreateError` as atomic_flag if you prefer.
> 
> Jie Yu wrote:
> > The second change is to optimize the creation. I thought this was 
> necessary because we will be calling create for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that create call, I thought it would be 
> better to reuse the initialized structures. 
> 
> Why we will be calling create for every URI fetch in a image pull? Can we 
> save the uri::Fetcher instance? Eventually, there'll be only one uri::Fetcher 
> at the top level (similar to the Fetcher right now).
> 
> Jojy Varghese wrote:
> One reason I can think of is to allow operaters to install missing 
> plugings (say hdfs) without having to restart. Let me know if that use case 
> is invalid.
> 
> Jie Yu wrote:
> Even though, i would rather add interfaces to allow us to update an 
> existing Fetcher, instead of creating a Fetcher each time we pull an image.

I have updated the patch to remove the optimization part based on discussion 
above. thanks!


- Jojy


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


On Jan. 25, 2016, 9:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 25, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41283: Introduced an Executor Library based on the new executor HTTP API.

[Vinod Kone]

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




src/executor/executor.cpp (line 274)


Can you add a comment here that you are making 2 persistent connections 
here, one for subscribe call (and its streaming response) and another for 
nonSubscribe calls.



src/executor/executor.cpp (lines 286 - 288)


formatting:

disconnected(subscribe,
 connection1.isFailed()
   ? connection1.failure()
   : "");
   
Why 'Subscribe' in quotes and not Non-subscribe below?



src/executor/executor.cpp (line 290)


no need for `else`. just make this a `if` block.



src/executor/executor.cpp (lines 291 - 294)


see formatting above.



src/executor/executor.cpp (line 299)


Maybe mention when state transitions from/to CONNECTED/DISCONNECTED?



src/executor/executor.cpp (line 308)


ditto. why quotes?



src/executor/executor.cpp (line 310)


s/Subscribe/subscribe/

Also have this comment up at #291?

More importantly, didn't we decide to close both connections if any of them 
fails/disconnects? I don't see that code in this patch?



src/executor/executor.cpp (line 318)


no need for quotes around Subscribe.

s/Subscribe/subscribe/



src/executor/executor.cpp (line 342)


shouldn't this be done inside `close()` ?



src/executor/executor.cpp (line 361)


dont you want to set `subscribe` and `nonSubscribe` to None()?



src/executor/executor.cpp (line 368)


Why is this outside the `if (state == CONNECTED)` block? I'm guessing you 
don't want to fire multiple delays one for subscribe disconnection and 
nonSubscribe disconnection for example?

If yes, you might just want to do the below at the top of this method.

```
if (state == DISCONNECTED) {
  return;
}
```

and merge this if block with the one at #352.


- Vinod Kone


On Jan. 22, 2016, 1:36 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41283/
> ---
> 
> (Updated Jan. 22, 2016, 1:36 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-3550
> https://issues.apache.org/jira/browse/MESOS-3550
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces the implementation for the executor library. 
> 
> This uses the new HTTP Connection interface to ensure calls are properly 
> pipelined.
> 
> `connected` -> Callback invoked when a TCP connection is established with the 
> agent for the `Subscribe` call.
> `disconnected` -> When the `Subscribe` TCP connection is interrupted possibly 
> due to an agent restart.
> `received` -> When the executor receives events from the agent upon 
> subscribing.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d23e35001078a86775bd9b76baa207ecb9dab7e1 
>   src/executor/executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41283/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 42735: Make commit-msg hook portable.

[Ian Downes]

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



Can you please fix the other bash Linux-isms in this patch while you're at it?
```
[1350][idownes:mesos]$ git grep "!\/bin\/bash"
support/atexit.sh:#!/bin/bash
support/coverage.sh:#!/bin/bash
support/docker_build.sh:#!/bin/bash
support/hooks/commit-msg:#!/bin/bash
support/release.sh:#!/bin/bash
support/tag.sh:#!/bin/bash
support/vote.sh:#!/bin/bash
```

- Ian Downes


On Jan. 25, 2016, 12:07 p.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42735/
> ---
> 
> (Updated Jan. 25, 2016, 12:07 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Ian Downes.
> 
> 
> Bugs: MESOS-4502
> https://issues.apache.org/jira/browse/MESOS-4502
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make commit-msg hook portable.
> 
> 
> Diffs
> -
> 
>   support/hooks/commit-msg d173dfdf380b08a9b349589dfd33c53b3cdccc60 
> 
> Diff: https://reviews.apache.org/r/42735/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Review Request 42739: Accounted for reserved resources in the quota guarantee check.

[Michael Park]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
65c7e6b15c5308c0910667e1b12f39b21293a316 
  src/tests/hierarchical_allocator_tests.cpp 
b1cb955b7eb1213c7ba4a9c5181545bb49154f06 

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


Testing
---


Thanks,

Michael Park

Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Greg Mann]

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

Review request for mesos, Jie Yu, Michael Park, and Neil Conway.


Repository: mesos


Description
---

Added checks for presence of `ReservationInfo.principal`.

The `ReservationInfo.principal` field was recently made `optional`. During the 
deprecation cycle, requests that do not have the field set are invalidated and 
rejected. However, there remain a couple places in the code that assume this 
field is always set. This patch uses `has_principal()` before this field is 
accessed to ensure safety.


Diffs
-

  src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 

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


Testing
---

`make check`


Thanks,

Greg Mann

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jojy Varghese]

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

(Updated Jan. 25, 2016, 9:19 p.m.)


Review request for mesos and Jie Yu.


Changes
---

removed optimization of plugin creation.


Repository: mesos


Description
---

Since fetcher plugins are not dynamically pluggable today, simplified the
factory method to use the statically intitalized plugins. This would be
useful when Fetchers are created multiple times.


Diffs (updated)
-

  src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 41959: Statically initializing fetcher plugins.

[Jojy Varghese]

> On Jan. 25, 2016, 5:12 p.m., Jie Yu wrote:
> > I am wondering if you do see performance issue regarding this? We typically 
> > don't optimize the code too much until it becomes a problem. Also, we try 
> > to avoid global variable dependencies (i.e., static bool isCreateError in 
> > this case).
> 
> Jojy Varghese wrote:
> Jie, thanks for looking at this. 
> 
> This change is two parts - one is simply replacing the initialization of 
> `creators` hashmap. Initialization using universal `{` initialization is the 
> preferred way in c++11 that calling `put`. 
> 
> The second change is to optimize the creation. I thought this was 
> necessary because we will be calling `create` for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that `create` call, I thought it would be 
> better to reuse the initialized structures. 
> 
> We can remove the 2nd part (optimization) if you like.
> 
> Jojy Varghese wrote:
> I could make the static flag `isCreateError` as atomic_flag if you prefer.
> 
> Jie Yu wrote:
> > The second change is to optimize the creation. I thought this was 
> necessary because we will be calling create for every URI fetch in a image 
> pull and its dependencies(or layers). As the information to create the 
> fetcher does not change for each of that create call, I thought it would be 
> better to reuse the initialized structures. 
> 
> Why we will be calling create for every URI fetch in a image pull? Can we 
> save the uri::Fetcher instance? Eventually, there'll be only one uri::Fetcher 
> at the top level (similar to the Fetcher right now).

One reason I can think of is to allow operaters to install missing plugings 
(say hdfs) without having to restart. Let me know if that use case is invalid.


- Jojy


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


On Jan. 25, 2016, 9:19 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41959/
> ---
> 
> (Updated Jan. 25, 2016, 9:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since fetcher plugins are not dynamically pluggable today, simplified the
> factory method to use the statically intitalized plugins. This would be
> useful when Fetchers are created multiple times.
> 
> 
> Diffs
> -
> 
>   src/uri/fetcher.cpp dfda732348fec3b686cf82b55ad94fda4829469b 
> 
> Diff: https://reviews.apache.org/r/41959/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jie Yu]

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



Haven't looked at the tests yet. Will do another pass once the existing issues 
are addressed. Thanks Jojy for doing this. This is really useful!


src/common/command_utils.hpp (line 32)


Can we remove these namespaces? I found the following less verbose and 
readable:
```
command::tar
command::untar
command::sha512
```
than
```
command::archive::tar
command::archive::untar
command::digest::sha512
```



src/common/command_utils.hpp (line 34)


'{' in the next line.



src/common/command_utils.hpp (line 50)


inputFilePath is a little confusing to readers because it can be a 
directory as well.

How about renaming it to 'input' since the Path part if pretty much 
redundent as the type is Path.



src/common/command_utils.hpp (line 51)


Ditto.

s/outputFilePath/output/



src/common/command_utils.hpp (line 52)


Hum, you use 'directory' in the cpp file while using a different name here. 
Can you use 'directory' through out?



src/common/command_utils.hpp (line 63)


Ditto. s/inputFilePath/input/



src/common/command_utils.hpp (line 64)


Ditto. s/changeToDirectory/directory/



src/common/command_utils.hpp (lines 70 - 74)


Can you separate the patches for sha512? We try to avoid big patches.



src/common/command_utils.cpp (line 44)


I would s/launchCommand/launch/ since you've already had 'command' in 
namespace.

command::launch reads much more nicely than command::launchCommand



src/common/command_utils.cpp (line 46)


We try to avoid using Option> since the semantics is not clear 
between None() and an empty vector.

For this case, I would suggest we always use the argv version of subprocess 
to avoid escaping spaces.

```
static Future launch(
const string& path,
const vector& argv)
{
  ...
}
```



src/common/command_utils.cpp (line 70)


Any way to print the agv as well?



src/common/command_utils.cpp (lines 117 - 124)


See my comments below, no need for this.



src/common/command_utils.cpp (lines 127 - 144)


See my comments below. No need for this.



src/common/command_utils.cpp (lines 153 - 162)


I don't think this check is necessary. We are basically checking what 'tar' 
will check later. Can you remove it?



src/common/command_utils.cpp (lines 172 - 176)


Ditto my comment above. I don't think the check performed in 
`addChangeDirectoryFlag` is necessary.  'tar' will validate that as well. Let's 
keep the function in this file simple (just shelling out).

No need to the additional helper function here. We also avoid using non 
const references if possible.



src/common/command_utils.cpp (lines 191 - 193)


What will this print? the type which will an int? Can you introduce a 
helper method to stringify the enum?



src/common/command_utils.cpp (line 228)


Let's not introduce this helper method. You can just do:

```
return launch(path, argv)
  .then([]() { return Nothing(); });
```


- Jie Yu


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Greg Mann]

> On Jan. 25, 2016, 9:05 p.m., Neil Conway wrote:
> > src/common/resources.cpp, line 66
> > 
> >
> > This is actually correct as written, no? i.e., if principal isn't set, 
> > `principal()` will return the empty string. Since an empty string isn't a 
> > legal principle, the comparison should just work, I believe.

I believe we allow the empty string as a valid principal.


- Greg


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


On Jan. 25, 2016, 9:02 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Jan. 25, 2016, 9:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. During 
> the deprecation cycle, requests that do not have the field set are 
> invalidated and rejected. However, there remain a couple places in the code 
> that assume this field is always set. This patch uses `has_principal()` 
> before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42746: Updated the allocator to offer revocable resources beyond quota.

[Michael Park]

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

(Updated Jan. 25, 2016, 10:08 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Simplified the condition for non-`USAGE_SLACK` resources.


Repository: mesos


Description
---

This patch updates the allocator internally such that we offer revocable
resources tagged as `RevocableInfo::EXTRA` to a role that has quota
enabled and its quota has been satisfied.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
65c7e6b15c5308c0910667e1b12f39b21293a316 
  src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
  src/tests/hierarchical_allocator_tests.cpp 
b1cb955b7eb1213c7ba4a9c5181545bb49154f06 

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


Testing
---


Thanks,

Michael Park

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?

I think it wont work because `argv` version uses the command passed to it as 
name of the binary to `execve` and the `non-argv` one uses `sh` as the binary. 
In fact i have tried using `argv` version of subprocess for `sha512` and it 
doesnt work.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
> 
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it 
> as name of the binary to `execve` and the `non-argv` one uses `sh` as the 
> binary. In fact i have tried using `argv` version of subprocess for `sha512` 
> and it doesnt work.
> 
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same 
> underlying mecheniam (os::execvpe) to exec the process. Why one works and the 
> other one does not?
> 
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", 
> command} ` as arguments to the command. Thats what I understood from L275 in 
> `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am 
> missing something.

Reading more code, I believe the difference is because of the PATH of some 
commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 42750: Used `std::any_of` instead of `std::count_if` when validating IDs.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

This makes the intent slightly clearer. In principle it should save a
few cycles as well, but nothing significant. Also, clarify the name of
a helper function.


Diffs
-

  src/master/validation.cpp f2bc1bad79e3b0812c019be3774cd65b58ea2d07 

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


Testing
---


Thanks,

Neil Conway

Review Request 42751: Tweaked some resource test cases.

[Neil Conway]

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

Review request for mesos.


Repository: mesos


Description
---

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs
-

  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 

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


Testing
---


Thanks,

Neil Conway

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```

I think we need both overloads of `subprocess` because `tar` would use the 
`argv` version and command like `sha512` would use the other one. I have the 
`Option` here to differentiate between the two.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, lines 153-162
> > 
> >
> > I don't think this check is necessary. We are basically checking what 
> > 'tar' will check later. Can you remove it?

The only reason I have these validations before we launch the process is to 
avoid launching the process if basic validations fail.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jie Yu]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, lines 153-162
> > 
> >
> > I don't think this check is necessary. We are basically checking what 
> > 'tar' will check later. Can you remove it?
> 
> Jojy Varghese wrote:
> The only reason I have these validations before we launch the process is 
> to avoid launching the process if basic validations fail.

That being said, let's try to avoid optimizing (usually means complexity) 
before there's data showing that it's a problem.


- Jie


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
> 
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it 
> as name of the binary to `execve` and the `non-argv` one uses `sh` as the 
> binary. In fact i have tried using `argv` version of subprocess for `sha512` 
> and it doesnt work.
> 
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same 
> underlying mecheniam (os::execvpe) to exec the process. Why one works and the 
> other one does not?

the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", 
command} ` as arguments to the command. Thats what I understood from L275 in 
`3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am 
missing something.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42662: Added common command utils file.

[Jie Yu]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
> 
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it 
> as name of the binary to `execve` and the `non-argv` one uses `sh` as the 
> binary. In fact i have tried using `argv` version of subprocess for `sha512` 
> and it doesnt work.

I don't understand. Both the argv and non-argv version use the same underlying 
mecheniam (os::execvpe) to exec the process. Why one works and the other one 
does not?


- Jie


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41911: Added a test case for corrupt packets.

[Cong Wang]

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

(Updated Jan. 25, 2016, 11:01 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Address review comments and rebase


Summary (updated)
-

Added a test case for corrupt packets.


Repository: mesos


Description (updated)
---

Added a test case for corrupt packets.


Diffs (updated)
-

  src/tests/containerizer/port_mapping_tests.cpp 
182fe9217a5da9af603d6f9c203a1689eff4ca1b 

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


Testing
---

make check


Thanks,

Cong Wang

Re: Review Request 42754: Added support for labels to resource reservations.

[Neil Conway]

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

(Updated Jan. 25, 2016, 11:02 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.


Diffs
-

  include/mesos/mesos.proto 96b911fb370223933df52f9370897871827d2247 
  include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a54411 
  include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9 
  include/mesos/v1/mesos.proto 0501dfa27ed610666226953591a902eac4c295f8 
  src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
  src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172 
  src/tests/mesos.hpp 5a737a6490060b0194db097990b327c9921221f4 
  src/tests/reservation_endpoints_tests.cpp 
35c093567b07a11ca6eee85e2ff91894a460a7af 
  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
  src/v1/mesos.cpp 9264752c6b82eaa844ce356b879f92d562ed4e45 
  src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 

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


Testing
---

make check


Thanks,

Neil Conway

Review Request 42755: Added documentation for labeled reserved resources.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Added documentation for labeled reserved resources.


Diffs
-

  docs/reservation.md 8d2d33a6518c73542cbfb3a5ee36da1c00c6ff1a 

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


Testing
---


Thanks,

Neil Conway

Review Request 42754: Added support for labels to resource reservations.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Labels are free-form key-value pairs that can be used to associate
metadata with reserved resources.


Diffs
-

  include/mesos/mesos.proto 96b911fb370223933df52f9370897871827d2247 
  include/mesos/type_utils.hpp 16f9cda83edd5064527b89e94392a7a54411 
  include/mesos/v1/mesos.hpp b294f022345dc892c0581622c5b389efbde911a9 
  include/mesos/v1/mesos.proto 0501dfa27ed610666226953591a902eac4c295f8 
  src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
  src/common/type_utils.cpp 76f48f6a1f5467db032ded8acd296d03353b4172 
  src/tests/mesos.hpp 5a737a6490060b0194db097990b327c9921221f4 
  src/tests/reservation_endpoints_tests.cpp 
35c093567b07a11ca6eee85e2ff91894a460a7af 
  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 
  src/v1/mesos.cpp 9264752c6b82eaa844ce356b879f92d562ed4e45 
  src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 40851: Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.

[M Lawindi]

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

(Updated Jan. 25, 2016, 11:21 p.m.)


Review request for mesos, Alex Naparu, Dario Bazan, Daniel Pravat, Alex 
Clemmer, and M Lawindi.


Summary (updated)
-

Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.


Repository: mesos


Description (updated)
---

Windows:[1/2] Updated zookeeper-3.4.5.patch to fix VS2015 build.


Diffs (updated)
-

  3rdparty/zookeeper-3.4.5.patch 3ca180d0c81f5de521ada7fb6c1c248a871ab2da 

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


Testing
---

- Applied patch to original zookeeper 3.4.5
- Successfully opened 2015 solution in VS2015
- Cli and Zookeeper build successfully


Thanks,

M Lawindi

Re: Review Request 42750: Used `std::any_of` instead of `std::count_if` when validating IDs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42750]

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

- Mesos ReviewBot


On Jan. 25, 2016, 10:49 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42750/
> ---
> 
> (Updated Jan. 25, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This makes the intent slightly clearer. In principle it should save a
> few cycles as well, but nothing significant. Also, clarify the name of
> a helper function.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp f2bc1bad79e3b0812c019be3774cd65b58ea2d07 
> 
> Diff: https://reviews.apache.org/r/42750/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 42662: Added common command utils file.

[Jojy Varghese]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
> 
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it 
> as name of the binary to `execve` and the `non-argv` one uses `sh` as the 
> binary. In fact i have tried using `argv` version of subprocess for `sha512` 
> and it doesnt work.
> 
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same 
> underlying mecheniam (os::execvpe) to exec the process. Why one works and the 
> other one does not?
> 
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", 
> command} ` as arguments to the command. Thats what I understood from L275 in 
> `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am 
> missing something.
> 
> Jojy Varghese wrote:
> Reading more code, I believe the difference is because of the PATH of 
> some commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.
> 
> Jie Yu wrote:
> execvpe will search PATH as well. I don't understand why sh is able to 
> find the executable while execvpe cannot.

Reading further, i think you are right. There shouldnt be any difference. I 
will update the patch to use `argv`. I think when i tried earlier, I was not 
passing all the args to the exec.


- Jojy


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 41911: Added a test case for corrupt packets

[Cong Wang]

> On Jan. 20, 2016, 6:12 p.m., Ian Downes wrote:
> > src/tests/containerizer/port_mapping_tests.cpp, line 1027
> > 
> >
> > This function is doing a lot, both constructing the packet, opening a 
> > socket and sending the packet. What about splitting this functionality? One 
> > function to construct, another to open/send/close?

I tried, but both constructing the packet and sending the packet require almost 
same parameters, so I am not sure if splitting in this way really improves 
anything here.


- Cong


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


On Jan. 14, 2016, 8:03 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41911/
> ---
> 
> (Updated Jan. 14, 2016, 8:03 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add a test case to ensure no corrupt packet could be delivered to application.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/port_mapping_tests.cpp 
> e3aea53468fa00374320a8b89bdbb64f38e44b01 
> 
> Diff: https://reviews.apache.org/r/41911/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 42662: Added common command utils file.

[Jie Yu]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.

Why can't you use the argv version for sha512 as well?


- Jie


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42733: Added checks for presence of `ReservationInfo.principal`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42733]

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

- Mesos ReviewBot


On Jan. 25, 2016, 9:02 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42733/
> ---
> 
> (Updated Jan. 25, 2016, 9:02 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Michael Park, and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added checks for presence of `ReservationInfo.principal`.
> 
> The `ReservationInfo.principal` field was recently made `optional`. During 
> the deprecation cycle, requests that do not have the field set are 
> invalidated and rejected. However, there remain a couple places in the code 
> that assume this field is always set. This patch uses `has_principal()` 
> before this field is accessed to ensure safety.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 0fcf86014ab5c9908a1cdb3a57b7c5e70acd7737 
>   src/master/master.cpp 9ee56277c8a472be9e683d5db505becfb5f7c422 
>   src/v1/resources.cpp 126e5a2f567d2e281da3f99bc485f7960567eee5 
> 
> Diff: https://reviews.apache.org/r/42733/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42662: Added common command utils file.

[Jie Yu]

> On Jan. 25, 2016, 9:45 p.m., Jie Yu wrote:
> > src/common/command_utils.cpp, line 46
> > 
> >
> > We try to avoid using Option> since the semantics is not 
> > clear between None() and an empty vector.
> > 
> > For this case, I would suggest we always use the argv version of 
> > subprocess to avoid escaping spaces.
> > 
> > ```
> > static Future launch(
> > const string& path,
> > const vector& argv)
> > {
> >   ...
> > }
> > ```
> 
> Jojy Varghese wrote:
> I think we need both overloads of `subprocess` because `tar` would use 
> the `argv` version and command like `sha512` would use the other one. I have 
> the `Option` here to differentiate between the two.
> 
> Jie Yu wrote:
> Why can't you use the argv version for sha512 as well?
> 
> Jojy Varghese wrote:
> I think it wont work because `argv` version uses the command passed to it 
> as name of the binary to `execve` and the `non-argv` one uses `sh` as the 
> binary. In fact i have tried using `argv` version of subprocess for `sha512` 
> and it doesnt work.
> 
> Jie Yu wrote:
> I don't understand. Both the argv and non-argv version use the same 
> underlying mecheniam (os::execvpe) to exec the process. Why one works and the 
> other one does not?
> 
> Jojy Varghese wrote:
> the `non-argv` one uses `sh` as the executable and passes ` {"sh", "-c", 
> command} ` as arguments to the command. Thats what I understood from L275 in 
> `3rdparty/libprocess/include/process/subprocess.hpp `. Let me know if I am 
> missing something.
> 
> Jojy Varghese wrote:
> Reading more code, I believe the difference is because of the PATH of 
> some commands. `tar`, `sh` are usually in the default PATH and shaXXX is not.

execvpe will search PATH as well. I don't understand why sh is able to find the 
executable while execvpe cannot.


- Jie


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


On Jan. 22, 2016, 5:38 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42662/
> ---
> 
> (Updated Jan. 22, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This common file is a good place to add common command line utilities like 
> tar,
> digests(sha256, sha512, etc). Currently this functionality is spread in the 
> code
> base and this change would enable all those call sites to be replaced with a
> common code.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 47d0a7c0fe73b9297cd7dde6086b5e6e9e1f9e4e 
>   src/Makefile.am 19bf3a7c2e43ca04ed6e6d506e052de5537f7c2f 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42662/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42751: Tweaked some resource test cases.

[Neil Conway]

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

(Updated Jan. 25, 2016, 10:59 p.m.)


Review request for mesos and Michael Park.


Repository: mesos


Description
---

We should check that two reservations with the same role but different
principals are considered distinct.


Diffs
-

  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 

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


Testing (updated)
---

make check


Thanks,

Neil Conway

Review Request 42752: Fixed some typos in test case comments.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Fixed some typos in test case comments.


Diffs
-

  src/tests/containerizer/port_mapping_tests.cpp 
182fe9217a5da9af603d6f9c203a1689eff4ca1b 
  src/tests/monitor_tests.cpp 2226458b59b4a279a92e1353bd61457a0018d2a9 
  src/tests/resources_tests.cpp 54a4fa88bfdcff3c0a7e89cbf3a1674c954b7f23 

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


Testing
---


Thanks,

Neil Conway

Review Request 42753: Allowed `createLabel` to take an optional "value".

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

This better matches the underlying protobuf definition.


Diffs
-

  src/common/protobuf_utils.hpp 7280c9fe36726df6b02ff468c7bd5ecedf5f5023 
  src/common/protobuf_utils.cpp 53324ab569751924f25290641d1a70da790c2104 

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


Testing
---


Thanks,

Neil Conway

Review Request 42757: Split os::memory() out into platform specific files.

[David Forsythe]

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

Review request for mesos and Ian Downes.


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


Repository: mesos


Description
---

Split os::memory() out into platform specific files.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/freebsd.hpp 
10346dea8d658ea0a9b658a1d72eee8cc3056da8 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/linux.hpp 
f8a4d5ae12cfc2d2bd4a24226e200e1aabe0b8a8 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp 
cc9bfe8942db7e62f5d415fc500b78adf2ac9759 
  3rdparty/libprocess/3rdparty/stout/include/stout/os/sunos.hpp 
0e897ea15d6b3ed76a8a264726db70980bb9517a 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
74af0077a39ef4cfa636b0b9e0c6b93eabc04bc8 

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


Testing
---

gmake check on FreeBSD and Ubuntu.


Thanks,

David Forsythe

Re: Review Request 42674: Replaced busybox with alpine in docker tests.

[Timothy Chen]

> On Jan. 23, 2016, 5:40 p.m., Jie Yu wrote:
> > Can you add more context in the description? Is that because there are too 
> > many links in busybox, causing issues with docker with brfts backend snce 
> > brfts backend has a limit on links?
> > 
> > Also, can you do a sweep in the code base to make sure there's no 'busybox' 
> > anymore?
> 
> Guangya Liu wrote:
> Per Tim's above comment, he only wants to fix the test cases which are 
> pulling or using the busybox image itself but not some reference, such as 
> https://github.com/apache/mesos/blob/master/src/tests/containerizer/provisioner_docker_tests.cpp#L93
>  etc
> 
> Bernd Mathiske wrote:
> Why not use "alpine" in the reference-only cases as well?
> 
> Guangya Liu wrote:
> Agree, it would be good to use "alpine" in all places to keep consistent.

I'm not inclined in changing the reference only cases, since it's a valid image 
to use and we include other information from busybox in other cases to test 
parsing imaging, etc. We switch to Alpine only because the actual filesystem 
image is causing problems with the docker daemon in some configurations.


- Timothy


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


On Jan. 23, 2016, 1:26 a.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42674/
> ---
> 
> (Updated Jan. 23, 2016, 1:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced busybox with alpine in docker tests.
> Busybox is currently causing issues with Docker daemon with btrfs backed 
> (https://github.com/docker/docker/issues/16755), which is the default on 
> CentOS 7.
> Tested CentOS and Ubuntu 14.04 with alpine and all Docker tests passed.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 81ab3625a69644d9659d484f3c605aaa5a10b397 
>   src/tests/containerizer/docker_tests.cpp 
> 9583070d64e96f5a57db62b3fb69073ecc5b502b 
>   src/tests/health_check_tests.cpp 3606ce46bfa283ad0d5239fc25e02c5a9f8d1a53 
>   src/tests/hook_tests.cpp 152984b01069acd4cf195bfce58835f0304a97f2 
> 
> Diff: https://reviews.apache.org/r/42674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>