Re: Review Request 56270: Made the default executor launch multiple task groups.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56267, 56268, 56269, 56270]

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

- Mesos Reviewbot


On Feb. 3, 2017, 4:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56270/
> ---
> 
> (Updated Feb. 3, 2017, 4:45 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6296
> https://issues.apache.org/jira/browse/MESOS-6296
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If a task fails or is explicitly killed by the scheduler, the
> default on termination policy kills the particular task group.
> The executor commits suicide when there are no active tasks.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/default_executor_tests.cpp 
> e8e0aa2f0d2508de6db03685c70d1ed9c1da3659 
> 
> Diff: https://reviews.apache.org/r/56270/diff/
> 
> 
> Testing
> ---
> 
> make check. Also, modified an existing test to launch/kill multiple task 
> groups.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 56268: Made `kill()` not invoke `shutdown()` in the default executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This ensures that when we add support for the default executor
launch multiple task groups, kill for a particular task should
_only_ result in the killing of a task group and not the entire
executor as was the case currently.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/default_executor_tests.cpp e8e0aa2f0d2508de6db03685c70d1ed9c1da3659 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Review Request 56270: Made the default executor launch multiple task groups.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

If a task fails or is explicitly killed by the scheduler, the
default on termination policy kills the particular task group.
The executor commits suicide when there are no active tasks.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/default_executor_tests.cpp e8e0aa2f0d2508de6db03685c70d1ed9c1da3659 

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


Testing
---

make check. Also, modified an existing test to launch/kill multiple task groups.


Thanks,

Anand Mazumdar

Review Request 56267: Made `kill` not use pipelining in the default executor.

[Anand Mazumdar]

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

Review request for mesos and Vinod Kone.


Bugs: mesos-6296
https://issues.apache.org/jira/browse/mesos-6296


Repository: mesos


Description
---

The previous code used to pipeline all the `KILL_NESTED_CONTAINER`
calls on the same connection. This change removes this and invokes
`post` for each child container. This simplifies the code and
makes it more readable.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 56251: Tightened assertions in sorter and allocator.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [56251, 56250, 56249, 56248, 56247, 56246, 56245]

Failed command: python support/apply-reviews.py -n -r 56251

Error:
2017-02-03 02:14:29 URL:https://reviews.apache.org/r/56251/diff/raw/ 
[2479/2479] -> "56251.patch" [1]
error: patch failed: src/master/allocator/mesos/hierarchical.cpp:432
error: src/master/allocator/mesos/hierarchical.cpp: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/16959/console

- Mesos Reviewbot


On Feb. 2, 2017, 10:59 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56251/
> ---
> 
> (Updated Feb. 2, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also replaced a mistaken comment.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 1d54f4c27428385bbcfc8e8bfb35aeea3a2cff7a 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 5681a5d78a7bdde820c3a8633d742d9d6412f1c7 
> 
> Diff: https://reviews.apache.org/r/56251/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Benjamin Mahler]

> On Jan. 30, 2017, 8:34 p.m., Michael Park wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 387-391
> > 
> >
> > Not yours, but do you know what's going on here?
> > The comment doesn't seem to match the code.

Hm.. did you interpret 'delete' to mean clear? The 'delete' is deferred until 
expire.


> On Jan. 30, 2017, 8:34 p.m., Michael Park wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1446
> > 
> >
> > Did you mean `const Owned&` here?
> > 
> > I'd prefer to not introduce more places where `Owned` is being used 
> > like a `Shared`.
> > 
> > or maybe even:
> > 
> > ```cpp
> > Sorter& frameworkSorter = *frameworkSorters.at(role);
> > ```

Whoops, yes I meant `const Owned&` as done on the other lookups.


- Benjamin


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


On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated Jan. 25, 2017, 2:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Benjamin Mahler]

> On Jan. 30, 2017, 3:31 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 128
> > 
> >
> > nit: This could be set in the member declaration.

Hm.. is that something we do? Most of our code uses the initializer list AFAICT.


> On Jan. 30, 2017, 3:31 p.m., Benjamin Bannier wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 237-241
> > 
> >
> > @gyliu: I like introducing `Framework` as a helper doing extraction 
> > from a `FrameworkInfo` (this is expanded on in r/55870).
> > 
> > @bmahler: `insert` also takes an `initializer_list`, i.e.,
> > 
> > frameworks.insert({frameworkId, Framework(frameworkInfo)});
> > 
> > Otherwise please include ``.

Will use an initializer list, thanks!


- Benjamin


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


On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated Jan. 25, 2017, 2:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55910: Prevent unintended mutation in the allocator.

[Benjamin Mahler]

> On Jan. 27, 2017, 7:13 a.m., Guangya Liu wrote:
> > src/master/allocator/mesos/hierarchical.cpp, lines 237-241
> > 
> >
> > Just a nit here: It seems a bit strange to me here of getting roles as 
> > following:
> > 1) Construct `frameworks`
> > 2) Get related framework info
> > 3) Get related framework role
> > 
> > How about keep the logic as before but put the logic of construct of 
> > `frameworks` to #275 here?

I'm not sure why it would strange to insert the framework before updating the 
sorters, can you clarify?


- Benjamin


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


On Jan. 25, 2017, 2:40 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55910/
> ---
> 
> (Updated Jan. 25, 2017, 2:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, a lof of code in the allocator makes use of the `[]`
> operator to access the agents, frameworks and sorters, which
> can lead to subtle bugs where insertion was unintended.
> 
> With this change, a few const functions can be marked as such.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 9b66c23f26b37c02ed850c06c4518ea99078b02d 
>   src/master/allocator/mesos/hierarchical.cpp 
> c2211be7458755aeb91ef078e4bfe92ac474044a 
> 
> Diff: https://reviews.apache.org/r/55910/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55868: Cleanups to the allocator tests.

[Benjamin Mahler]

> On Jan. 26, 2017, 5:14 a.m., Guangya Liu wrote:
> > src/tests/hierarchical_allocator_tests.cpp, line 1950
> > 
> >
> > Any reason you want to update here?

This was a case where it was safe to use EXPECT instead of ASSERT.


- Benjamin


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


On Jan. 24, 2017, 2:31 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55868/
> ---
> 
> (Updated Jan. 24, 2017, 2:31 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was necessary to greatly simplify the changes needed to the
> allocator tests as we introduce support for multi-role frameworks.
> 
> The main improvement here is to establish and use equality on the
> `Allocation` struct, which makes the tests more readable and avoids
> the manual probing of the allocation structure across all the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 1edd0ecc8a93cd41532e1cf3641f67c780ab23a5 
> 
> Diff: https://reviews.apache.org/r/55868/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55863: Introduce a helper for injecting AllocationInfo into offer operations.

[Benjamin Mahler]

> On Jan. 30, 2017, 1:23 a.m., Michael Park wrote:
> > src/common/protobuf_utils.cpp, lines 332-335
> > 
> >
> > Is there a reason why we don't just mutate it in-place?
> > ```cpp
> > void adjustOfferOperation(
> > Offer::Operation* operation,
> > const Resource::AllocationInfo& allocationInfo);
> > ```

Sounds good to me.


> On Jan. 30, 2017, 1:23 a.m., Michael Park wrote:
> > src/common/protobuf_utils.cpp, lines 340-341
> > 
> >
> > I think it'd be clearer for us to write:
> > 
> > ```cpp
> > foreach (TaskInfo& task, *result.launch().mutable_task_infos()) {
> >   // ...
> > }
> > ```

Now I remember why I avoided this, note that it has to be mutable the whole way 
through:

```
foreach (TaskInfo& task,
 *operation->mutable_launch()->mutable_task_infos()) {
  // ...
}
```

I'll work around this by storing the launch and other operations into variables.


> On Jan. 30, 2017, 1:23 a.m., Michael Park wrote:
> > src/common/protobuf_utils.cpp, lines 343-348
> > 
> >
> > We basically do this for every repeated resources field within 
> > `Offer::Operation`. Can we introduce a function similar to 
> > `adjustOfferOperation`?
> > 
> > ```cpp
> > void adjustResources(
> > RepeatedPtrField* resources,
> > const Resource::AllocationInfo& allocationInfo);
> > ```
> > 
> > The `injectAllocationInfo` helper which operates on `FrameworkInfo`
> > I think should just be:
> > 
> > ```cpp
> >   auto injectAllocationInfo = [](
> >   RepeatedPtrField* resources,
> >   const FrameworkInfo& frameworkInfo)
> >   {
> > if (protobuf::frameworkHasCapability(
> >   frameworkInfo,
> >   FrameworkInfo::Capability::MULTI_ROLE) {
> >   return;  
> > }
> > 
> > Resource::AllocationInfo allocationInfo;
> > allocationInfo.set_role(frameworkInfo.role);
> > adjustResources(resources, allocationInfo);
> >   };
> > ```

Added a lambda to simplify this implementation. Will look into your suggestion 
for the subsequent patch.


- Benjamin


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


On Jan. 23, 2017, 10:59 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55863/
> ---
> 
> (Updated Jan. 23, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an `AllocationInfo`.
> 
> This introduces a function which allows the master to inject the
> offer's `AllocationInfo` into the operation's resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
>   src/common/protobuf_utils.cpp dd20759affe3adf2867a33bf875c9ee82862038d 
>   src/tests/protobuf_utils_tests.cpp bc2a3d0ebe544a58d0de8f2f7174c170113ddf2e 
> 
> Diff: https://reviews.apache.org/r/55863/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 55863: Introduce a helper for injecting AllocationInfo into offer operations.

[Benjamin Mahler]

> On Jan. 28, 2017, 11:02 a.m., Guangya Liu wrote:
> > src/common/protobuf_utils.cpp, lines 345-347
> > 
> >
> > Since we do not support one `Resources` store a mix of allocated and 
> > unallocated resources, how about optimize this a bit as:
> > 
> > ```
> > if (resource->has_allocation_info()) {
> >   break;
> > }
> > 
> > resource->mutable_allocation_info()->CopyFrom(allocationInfo);
> > ```
> > 
> > Ditto for here and everywhere

Well, we do support mixing in Resources, but we likely do not want to. I would 
like to have a stronger invariant that callers can't store a mix of allocated 
and unallocated resources, since I don't see a good reason for it and it's 
error prone.


- Benjamin


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


On Jan. 23, 2017, 10:59 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55863/
> ---
> 
> (Updated Jan. 23, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael 
> Park.
> 
> 
> Bugs: MESOS-6967
> https://issues.apache.org/jira/browse/MESOS-6967
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, `Resource` did not contain `AllocationInfo`. So for
> backwards compatibility with old schedulers and tooling, we must
> allow operations to contain `Resource`s without an `AllocationInfo`.
> 
> This introduces a function which allows the master to inject the
> offer's `AllocationInfo` into the operation's resources.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.hpp faa7e2a759fd2b1ce5def662679f573ec6fefd28 
>   src/common/protobuf_utils.cpp dd20759affe3adf2867a33bf875c9ee82862038d 
>   src/tests/protobuf_utils_tests.cpp bc2a3d0ebe544a58d0de8f2f7174c170113ddf2e 
> 
> Diff: https://reviews.apache.org/r/55863/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 56251: Tightened assertions in sorter and allocator.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Also replaced a mistaken comment.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
1d54f4c27428385bbcfc8e8bfb35aeea3a2cff7a 
  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56249: Cleaned up code style slightly in DRF sorter.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Cleaned up code style slightly in DRF sorter.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
5681a5d78a7bdde820c3a8633d742d9d6412f1c7 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56248: Replaced use of ".get()" in master quota tests with "->".

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Replaced use of ".get()" in master quota tests with "->".


Diffs
-

  src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56247: Switched to implicit roles in master quota tests.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Using an explicit role list is deprecated; this change will also make it
easier to write test cases for hierarchical roles.


Diffs
-

  src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56245: Fixed typos in name of test suite.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Fixed typos in name of test suite.


Diffs
-

  src/tests/role_tests.cpp 0d0f1a94ff9aaec6ea04280282a52ac88dc6b273 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 56246: Fixed typos.

[Neil Conway]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

Fixed typos.


Diffs
-

  src/authentication/cram_md5/authenticatee.cpp 
16291a1a44832a87f1bf3a0b8ca71454126ef169 
  src/launcher/fetcher.cpp a1dcced05a4438152b3d2746eea41daf54a0ee4b 
  src/master/validation.cpp f45b6446fe224d1507520bf600db887e2b224a29 
  src/slave/flags.cpp b48678bae8b3f5bab62710043eaca14ab8370183 
  src/tests/master_quota_tests.cpp b25f5911579c435549b9bc65994627414357dcb6 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 56195: Updated containerizer->launch path to close IOSwitchboard FDs on error.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1140)


Hum, this could cause as FD being closed twice.

Say `_launch` fails after `launcher->fork`. That means the subprocess call 
already close the parent end of the pipe. Here, you'll be closing it again.

I think the only elegant fix is to introduce file descriptor abstraction 
and use RAII to make sure the fd will be closed in error case.


- Jie Yu


On Feb. 2, 2017, 5:29 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56195/
> ---
> 
> (Updated Feb. 2, 2017, 5:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
> and Jie Yu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if the containizer launch path failed before actually
> launching the container, the FDs allocated to the container by the
> IOSwitchboard isolator would be leaked. This would lead to deadlock in
> the destroy path because the IOSwitchboard does not shutdown until the
> FDs it allocates to the container have been closed. Since the
> switchboard doesn't shutdown, the future returned by its 'cleanup()'
> function is never satisfied.
> 
> This commit makes sure to close the FDs under all failure cases in the
> launch path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4f0a773676da45fa40ad1ad9cdfab2a19249247d 
> 
> Diff: https://reviews.apache.org/r/56195/diff/
> 
> 
> Testing
> ---
> 
> Linux CentOS 7:
> ```
> GTEST_FILTER="" make -j check
> src/mesos-tests
> [--] Global test environment tear-down
> [==] 1477 tests from 167 test cases ran. (390804 ms total)
> [  PASSED  ] 1477 tests.
> ```
> 
> Mac OS X (El Capitan)
> ```
> GTEST_FILTER="" make -j check
> src/mesos-tests
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] ExamplesTest.V1JavaFramework
> [  FAILED  ] ExamplesTest.PythonFramework
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 55732: Added `--3way` option to apply-reviews.py.

[Michael Park]

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


Fix it, then Ship it!





support/apply-reviews.py (line 7)


I don't think this is necessary right? We already import `os`.



support/apply-reviews.py (line 123)


Is there something related to this patch that makes it not exist sometimes?



support/apply-reviews.py (line 326)


Let's add the `-3` short option as well.


- Michael Park


On Feb. 1, 2017, 10:45 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55732/
> ---
> 
> (Updated Feb. 1, 2017, 10:45 a.m.)
> 
> 
> Review request for mesos, Jason Lai, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-4119
> https://issues.apache.org/jira/browse/MESOS-4119
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This can help on some patches which includes conflicts that can be
> resolved by 3 way merge.
> 
> 
> Diffs
> -
> 
>   support/apply-reviews.py cdfdd58f9310f95b075c025048b6523fc1a4da65 
> 
> Diff: https://reviews.apache.org/r/55732/diff/
> 
> 
> Testing
> ---
> 
> With this fix, I was able to apply the chain in r/52534, which has a conflict 
> otherwise.
> 
> Bash log:
> ```
> 
> $ python ./support/apply-reviews.py -r 52534 -c --3way
> 2017-01-23 10:00:57 URL:https://reviews.apache.org/r/51027/diff/raw/ 
> [9830/9830] -> "51027.patch" [1]
> [3way 93791b1] Track allocation candidates to bound allocator.
>  Author: Jacob Janco 
>  2 files changed, 97 insertions(+), 54 deletions(-)
> 2017-01-23 10:00:59 URL:https://reviews.apache.org/r/52534/diff/raw/ 
> [2950/2950] -> "52534.patch" [1]
> error: patch failed: src/master/allocator/mesos/hierarchical.hpp:224
> Falling back to three-way merge...
> Applied patch to 'src/master/allocator/mesos/hierarchical.hpp' cleanly.
> [3way 6f24fe7] Dispatch filter expiration twice.
>  Author: Jacob Janco 
>  2 files changed, 32 insertions(+), 8 deletions(-)
>  ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53790, 54712, 54878, 53791]

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

- Mesos Reviewbot


On Feb. 2, 2017, 5:32 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Feb. 2, 2017, 5:32 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 56237: Add `syntax = "proto2"; ` to the rest of the files.

[Anthony Sottile]

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

Review request for mesos, Deshi Xiao, Yong Tang, and Zhitao Li.


Bugs: MESOS-5186 and MESOS-6138
https://issues.apache.org/jira/browse/MESOS-5186
https://issues.apache.org/jira/browse/MESOS-6138


Repository: mesos


Description
---

These files were identified with the following pipeline:

# Find all .proto files, then filter to those without a syntax line.
git ls-files -- '*.proto' | xargs grep -L 'syntax ='


Diffs
-

  3rdparty/stout/tests/protobuf_tests.proto 
229ac256b087d55ecc95636254261ecd12829187 

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


Testing
---


Thanks,

Anthony Sottile

Review Request 56238: Allow a newer version of google protobuf.

[Anthony Sottile]

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

Review request for mesos, Deshi Xiao, Yong Tang, and Zhitao Li.


Bugs: MESOS-5186 and MESOS-6138
https://issues.apache.org/jira/browse/MESOS-5186
https://issues.apache.org/jira/browse/MESOS-6138


Repository: mesos


Description
---

Allow a newer version of google protobuf.


Diffs
-

  src/python/interface/setup.py.in 037c2ec8e63f497f7029a847a7a0d7b72e6f36fa 
  src/python/protocol/setup.py.in 5dd4a0a0f67c4003b58319d0d6c7bb227e8a13a1 

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


Testing
---


Thanks,

Anthony Sottile

Review Request 56236: Add `syntax = "proto2"` to some files automatically.

[Anthony Sottile]

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

Review request for mesos, Deshi Xiao, Yong Tang, and Zhitao Li.


Bugs: MESOS-5186 and MESOS-6138
https://issues.apache.org/jira/browse/MESOS-5186
https://issues.apache.org/jira/browse/MESOS-6138


Repository: mesos


Description
---

I used this pipeline:

# Find all proto files with the 15-line license and add the syntax line
# after that
git grep -n 'limitations under the License' -- '*.proto' |
grep ':15' | cut -d':' -f1 |
xargs sed -i '16isyntax = "proto2";'


Diffs
-

  include/mesos/agent/agent.proto 775c14b70c94c90d31d0123f45188fd29651f9f4 
  include/mesos/allocator/allocator.proto 
9fc8baccb135c5aa58f64847307978139139a46e 
  include/mesos/appc/spec.proto 2dbb3504596dda229c42353098879350c3a7c960 
  include/mesos/authentication/authentication.proto 
a24d53512a8ec27e97cf543cd64129c8a096ac67 
  include/mesos/authorizer/acls.proto fd25e5449ad659ee0269cfc3f47b9939ac022fb4 
  include/mesos/authorizer/authorizer.proto 
8b860a3e8e0b1c660a8fefc97f10f5acc0501920 
  include/mesos/docker/spec.proto 734395cbde81bc4ff99f1310fc4b913beeb5b11f 
  include/mesos/docker/v1.proto 209fb4afe5e4ea1cc776bfe9cb36865c9b05aaf0 
  include/mesos/docker/v2.proto 9b5bfb5d03ae8115c1b20a8c640640d6dbaa257d 
  include/mesos/executor/executor.proto 
e746608176245bcabf265925111b8df9cab8ca55 
  include/mesos/fetcher/fetcher.proto 7f204e1a0b70ecdcd4247e1c0699585243a97b40 
  include/mesos/maintenance/maintenance.proto 
4afae5c6958a5da91b3e649b496b9757d951ca0c 
  include/mesos/master/master.proto a2228db8902c297f137b8106a7b3d2babbc35017 
  include/mesos/mesos.proto 53885cbc63ac6658a749da5e05bb2301392f84dd 
  include/mesos/module/hook.proto f7b91f813f7919098763790211d3b90aed0b8340 
  include/mesos/module/module.proto 1c6c4b0762f5a4e71cf64fd9bc859e380222b591 
  include/mesos/quota/quota.proto 78e12d56c4f41613ec6b731d86093f48a23b4509 
  include/mesos/scheduler/scheduler.proto 
5f4635d523286754a61aa99e18e79d6c1db9463f 
  include/mesos/slave/containerizer.proto 
c70d437ac3f8f813fcb71c04275cc8eeeb946065 
  include/mesos/slave/oversubscription.proto 
e7346089d5699194b761c81ab9610ad33d83ba55 
  include/mesos/state/state.proto 7a7d68e6abc85c0ead04f5771e878d10348f44b8 
  include/mesos/uri/uri.proto 55023b681665e712990d45ff4a1dbc4ccf64edbf 
  include/mesos/v1/agent/agent.proto a98acb7b0c66f7e17dcecf61ed22bfde70a33651 
  include/mesos/v1/allocator/allocator.proto 
093f18f554470b14905db157bcc1e33303423eaa 
  include/mesos/v1/executor/executor.proto 
754e62aee5f822526eb9661aa255444c3f84dd1d 
  include/mesos/v1/maintenance/maintenance.proto 
27f7c2b3460fe3a7e1a845cd962cb9979a9ccc0b 
  include/mesos/v1/master/master.proto cfdca7400d98233c6320eebd8784e4f51c43ebbd 
  include/mesos/v1/mesos.proto c4ca6dea787cfe4661c9f0d9afb770bceb1c2639 
  include/mesos/v1/quota/quota.proto 424e6b4bf0064d146cfeeb19de77f0cce67882b9 
  include/mesos/v1/scheduler/scheduler.proto 
096c76dfffe03c0e2d6abe84d438c396cc1b0be9 
  src/master/registry.proto eab9821b6d159d14bf62b347d6188608ae5c 
  src/messages/flags.proto 08406b69acaa52eba58e6091b9c8b3bc03981044 
  src/messages/log.proto 12c2d83d88bb5b282bc44d49655a67e671b12ba2 
  src/messages/messages.proto 7a2f37b78a8edcd372558f77f15e6b249742e321 
  src/messages/state.proto 857a88873681c9cd7263d0063e8f17f08c5cd64b 
  src/slave/containerizer/mesos/isolators/docker/volume/state.proto 
4c5b03c1a802c177f64ea8bb1e31fa58603991bb 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto 
4b3850037ec01969cabf16b94745c1802bf4de62 
  src/slave/containerizer/mesos/provisioner/docker/message.proto 
c93c7a92ec152bd9747a70392adfe6a0e863e839 

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


Testing
---


Thanks,

Anthony Sottile

Re: Review Request 54601: Replaced `int` with `int_fd` in stout.

[Alex Clemmer]

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




3rdparty/stout/include/stout/os/mktemp.hpp (line 44)


This also seems to be not a correct comparison? See the comment in 
`io::read` in https://reviews.apache.org/r/54602/



3rdparty/stout/include/stout/os/open.hpp (line 74)


This also seems to be not a correct comparison? See the comment in 
`io::read` in https://reviews.apache.org/r/54602/


- Alex Clemmer


On Jan. 10, 2017, 2:49 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54601/
> ---
> 
> (Updated Jan. 10, 2017, 2:49 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `int` with `int_fd` in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/net.hpp 
> 4803aeb2fc1c89d42a02c631ef0450d5053ea8a2 
>   3rdparty/stout/include/stout/os/mktemp.hpp 
> 2dfd35605eb4202a5475fe0e0d2f1fd27690a2de 
>   3rdparty/stout/include/stout/os/open.hpp 
> 2a357926860b1523c51f12c7edee2babe6afbfa3 
>   3rdparty/stout/include/stout/os/posix/socket.hpp 
> 041f00083c595c335146048c8685c4f96226b8e8 
>   3rdparty/stout/include/stout/os/read.hpp 
> d0b657db5b7dc0c1e11edc13fd6830a9f6273867 
>   3rdparty/stout/include/stout/os/socket.hpp 
> e9d9306e63aff2be083b4254fbf6f31c37edc424 
>   3rdparty/stout/include/stout/os/sunos.hpp 
> e0398d25c0a5d0b55934594dd59b2629957ab921 
>   3rdparty/stout/include/stout/os/touch.hpp 
> 84d49bb8adc2612e380f037fd42c47166d55f593 
>   3rdparty/stout/include/stout/os/windows/close.hpp 
> 9f1566bff19ee872418bce8a43a119c4f0a70a7c 
>   3rdparty/stout/include/stout/os/windows/fcntl.hpp 
> f331cd324bc609f5b8081fa86d66d9947daf04f6 
>   3rdparty/stout/include/stout/os/windows/fsync.hpp 
> e1c4868b02b320906f446af1d9371374e90651bc 
>   3rdparty/stout/include/stout/os/windows/ftruncate.hpp 
> a7f53ad2e8735b515590af84c0efce3edcc1bebf 
>   3rdparty/stout/include/stout/os/windows/read.hpp 
> 09190f97f33db5dfa023f937f8f2a4a62ed1ec15 
>   3rdparty/stout/include/stout/os/windows/sendfile.hpp 
> d6358cc02c1eea9298907da1f74eb7eeaeec7d21 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> 17e3d564564abebf1d558b7a7a277aef3c87e5ae 
>   3rdparty/stout/include/stout/os/windows/socket.hpp 
> c787341181da53abc5a3b2eadc76c85fef181310 
>   3rdparty/stout/include/stout/os/windows/write.hpp 
> 23488708ae366b8571bb8b4805f67d2054223fff 
>   3rdparty/stout/include/stout/os/write.hpp 
> 9bb9fbd4593866b6193a1e253e65852da0ff5ed5 
>   3rdparty/stout/include/stout/protobuf.hpp 
> 80cb20f40a7ddd4309d27973eef9fca9e4052b64 
>   3rdparty/stout/include/stout/windows.hpp 
> e641c46d033372e1b6c9f9c066b1ad4957d55088 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 5cd92545a49648e39e8eb7cf131895e9cfc97902 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> 22460842c0db5dc5b6effbc2bdfce043ed47db6d 
>   3rdparty/stout/tests/os/sendfile_tests.cpp 
> 17e10d92a0a004cb7ac83f4ff9c71844418a35b0 
>   3rdparty/stout/tests/os_tests.cpp 30735e28a26ff713469711d63538676ed4e327d9 
>   3rdparty/stout/tests/path_tests.cpp 
> 8275e38a503e64d242da6be0fe6bd0a0c21869a3 
> 
> Diff: https://reviews.apache.org/r/54601/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 54602: Replaced `int` with `int_fd` in libprocess.

[Alex Clemmer]

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




3rdparty/libprocess/src/io.cpp (line 222)


This comparison is definitely a bug. On Windows, when this `fd` is a pipe 
`HANDLE`, this comparison fails even for valid values of `fd`.

This can, among other things, cause the Windows Executor to hang 
indefinitely in some scenarios. For example, when we call `docker inspect`, we 
will probably end up writing more data to the pipe than the pipe has buffer, so 
the Executor will block until we `io::read` it. But since this comparison 
fails, we will never complete the read. Hence, the Executor hangs indefinitely.

This comparison can be made to work on Windows by making it properly check 
whether the `HANDLE` (using, _e.g._, `fd == 
os::WindowsFd(INVALID_HANDLE_VALUE)`) is invalid, but this obviously won't work 
on POSIX. In general, I suspect these comparisons are fraught, so I think we 
should consider removing them and having utility functions such as `fd.valid()` 
or something.



3rdparty/libprocess/src/io.cpp (line 283)


This also seems to be not a correct comparison? See the comment in 
`io::read`.



3rdparty/libprocess/src/process.cpp (line 1457)


This also seems to be not a correct comparison? See the comment in 
`io::read`.


- Alex Clemmer


On Jan. 10, 2017, 2:50 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54602/
> ---
> 
> (Updated Jan. 10, 2017, 2:50 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced `int` with `int_fd` in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> f5489dc66adb136d9f53f98ac64c3fbe7831a1c2 
>   3rdparty/libprocess/include/process/network.hpp 
> 8234765e23bb3d434da0b0f818fd42569d554ab8 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> a1054e788ef6a322901c262380aceab8192235ac 
>   3rdparty/libprocess/include/process/socket.hpp 
> 87966155aa21328db51796b2ae0a883054c00457 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> 74a4bef0706334b4d3c1040a08a8921fbc300344 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> 3bc7f1992d9c38dac2ec23d5bc57415f37d0318a 
>   3rdparty/libprocess/src/encoder.hpp 
> 1447d6f93c15b9f3d134507ecb0bda020a218a49 
>   3rdparty/libprocess/src/http.cpp b5c38ce89b73788f7446e6c44fd99da6478b064d 
>   3rdparty/libprocess/src/io.cpp 8aa3576a11ed5e998492b4020cb37a45ec708093 
>   3rdparty/libprocess/src/libev_poll.cpp 
> da2a78bd8aa4ed8c37bd2ca16d148b73112aa0e7 
>   3rdparty/libprocess/src/libevent_poll.cpp 
> 0803ba33622c86df38b3efd4f1e3197edf93a0af 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp 
> 57eaf4f607d0628db466cc1a139772eeeaa51136 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 0e292a8b0d470f4e199db08f09a0c863d73c 
>   3rdparty/libprocess/src/poll_socket.hpp 
> 89789e6bb91d79e4de1c4f4be3882df851845930 
>   3rdparty/libprocess/src/poll_socket.cpp 
> 93ca37f105527fb225574107480114ee5ac74c76 
>   3rdparty/libprocess/src/process.cpp 
> f475fe78f801924f70f51fdc4ab190c2dbecd656 
>   3rdparty/libprocess/src/socket.cpp b819503095261c77f42d6f20d1a4b2b6170fb4e1 
>   3rdparty/libprocess/src/subprocess.cpp 
> ad19b0896b4a2e9c60f573cc854c10c69e909e86 
>   3rdparty/libprocess/src/subprocess_posix.cpp 
> 19271414f145d23f50ac07570c48782819f382b4 
>   3rdparty/libprocess/src/subprocess_windows.cpp 
> 20cad52d4a4d7fc51487e150a849972eb19ed08e 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 466e343e6d775fe09debce119eae4fc4849b7006 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> a32d20e47f67d88bbe5928e0ddc129745c5f42e0 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 59c17692012ddfb540ecdd48560c73c42a15f061 
> 
> Diff: https://reviews.apache.org/r/54602/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 55901: Added support for command health checks to the default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [55900, 55901]

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

- Mesos Reviewbot


On Feb. 2, 2017, 5:02 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55901/
> ---
> 
> (Updated Feb. 2, 2017, 5:02 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent 
> huang, and Vinod Kone.
> 
> 
> Bugs: MESOS-6280
> https://issues.apache.org/jira/browse/MESOS-6280
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for command health checks to the default executor.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
>   src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
>   src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
>   src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 
> 
> Diff: https://reviews.apache.org/r/55901/diff/
> 
> 
> Testing
> ---
> 
> Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
> passes on Linux, but not on macOS.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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

(Updated Feb. 2, 2017, 5:32 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


Changes
---

Addressed review feedback.


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


Repository: mesos


Description
---

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-

  src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
  src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 

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


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 55901: Added support for command health checks to the default executor.

[Gastón Kleiman]

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

(Updated Feb. 2, 2017, 5:02 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Addressed feedback + rebase.


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


Repository: mesos


Description
---

Added support for command health checks to the default executor.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 
  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 
  src/tests/health_check_tests.cpp 8418cd91484fd26734de16255b37f3ebf574f5eb 

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


Testing
---

Introduced a new test: `HealthCheckTest.DefaultExecutorCmdHealthCheck`. It 
passes on Linux, but not on macOS.


Thanks,

Gastón Kleiman

Re: Review Request 55900: Improved style in `HealthChecker`.

[Gastón Kleiman]

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

(Updated Feb. 2, 2017, 4:54 p.m.)


Review request for mesos, Alexander Rukletsov, Anand Mazumdar, haosdent huang, 
and Vinod Kone.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Marked `_taskPid` in the constructor as const, to be consistent with the
other parameters.


Diffs (updated)
-

  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 

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


Testing
---

`make check` (macOS and Linux)


Thanks,

Gastón Kleiman

Re: Review Request 56218: Added some check tests for default executor.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56208, 56209, 56210, 56211, 56212, 56213, 56214, 56215, 
56216, 56217, 56218]

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

- Mesos Reviewbot


On Feb. 2, 2017, 9:58 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56218/
> ---
> 
> (Updated Feb. 2, 2017, 9:58 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
> 
> Diff: https://reviews.apache.org/r/56218/diff/
> 
> 
> Testing
> ---
> 
> Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 56144: Added unit test for http::Headers abstraction.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56116, 56117, 54537, 56144]

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

- Mesos Reviewbot


On Feb. 2, 2017, 8:40 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56144/
> ---
> 
> (Updated Feb. 2, 2017, 8:40 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
> Lin, and Timothy Chen.
> 
> 
> Bugs: MESOS-7051
> https://issues.apache.org/jira/browse/MESOS-7051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for http::Headers abstraction.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> b12849e506f4340b865840072e583dfddecaf02a 
> 
> Diff: https://reviews.apache.org/r/56144/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 55321: Introduced process::after.

[Gastón Kleiman]

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




3rdparty/libprocess/Makefile.am (line 237)


Shouldn't we update the cmake lists as well?


- Gastón Kleiman


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process::after.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 
> 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55321: Introduced process::after.

[Alexander Rukletsov]

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




3rdparty/libprocess/include/Makefile.am (lines 15 - 16)


Swap these.



3rdparty/libprocess/include/process/after.hpp (line 51)


We keep two copies of a shared pointer to `promise` in an `onDiscard` 
callback (one directly and one via `timer`). It seems that all code paths in 
`after` do clear `onDiscardCallbacks` and hence destruct the promise. But I'd 
have tests proving that rather than relying on my mental compiling abilities : 
).


- Alexander Rukletsov


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55321/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process::after.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 5e7fdd06ccbed50f248c81e9df1651a3702e7add 
>   3rdparty/libprocess/include/Makefile.am 
> 1d17edd933562849b35740f3935685c8eb154440 
>   3rdparty/libprocess/include/process/after.hpp PRE-CREATION 
>   3rdparty/libprocess/src/tests/after_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55321/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 55322: Used process::after instead of process::RateLimiter.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55322/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::after instead of process::RateLimiter.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
> 
> Diff: https://reviews.apache.org/r/55322/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 56195: Updated containerizer->launch path to close IOSwitchboard FDs on error.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [56195]

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

- Mesos Reviewbot


On Feb. 2, 2017, 5:29 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56195/
> ---
> 
> (Updated Feb. 2, 2017, 5:29 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gastón Kleiman, Gilbert Song, 
> and Jie Yu.
> 
> 
> Bugs: MESOS-7050
> https://issues.apache.org/jira/browse/MESOS-7050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, if the containizer launch path failed before actually
> launching the container, the FDs allocated to the container by the
> IOSwitchboard isolator would be leaked. This would lead to deadlock in
> the destroy path because the IOSwitchboard does not shutdown until the
> FDs it allocates to the container have been closed. Since the
> switchboard doesn't shutdown, the future returned by its 'cleanup()'
> function is never satisfied.
> 
> This commit makes sure to close the FDs under all failure cases in the
> launch path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 4f0a773676da45fa40ad1ad9cdfab2a19249247d 
> 
> Diff: https://reviews.apache.org/r/56195/diff/
> 
> 
> Testing
> ---
> 
> Linux CentOS 7:
> ```
> GTEST_FILTER="" make -j check
> src/mesos-tests
> [--] Global test environment tear-down
> [==] 1477 tests from 167 test cases ran. (390804 ms total)
> [  PASSED  ] 1477 tests.
> ```
> 
> Mac OS X (El Capitan)
> ```
> GTEST_FILTER="" make -j check
> src/mesos-tests
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] ExamplesTest.V1JavaFramework
> [  FAILED  ] ExamplesTest.PythonFramework
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 55322: Used process::after instead of process::RateLimiter.

[Alexander Rukletsov]

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


Ship it!




This will most likely fix https://issues.apache.org/jira/browse/MESOS-7036, 
mind mentioning the ticket here and updating the ticket?

Having said that, I'm curious whether we should work on a more general solution 
to prevent bugs like MESOS-7036 in the future. See the example test in 
MESOS-7036 for more information.

- Alexander Rukletsov


On Jan. 8, 2017, 7:49 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55322/
> ---
> 
> (Updated Jan. 8, 2017, 7:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::after instead of process::RateLimiter.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
> 
> Diff: https://reviews.apache.org/r/55322/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 56188: Removed all instances of '.Times(1)' from libprocess tests.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Feb. 1, 2017, 10:26 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56188/
> ---
> 
> (Updated Feb. 1, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using `.Times(1)` with `EXPECT_CALL` is redundant, as this
> is implied by default. This patch removes all such remaining
> occurrences from the libprocess tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> cd159b6dbde44d89bca76c6ec99cc22621638b2a 
> 
> Diff: https://reviews.apache.org/r/56188/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56187: Removed all instances of '.Times(1)' from Mesos tests.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Feb. 1, 2017, 10:26 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56187/
> ---
> 
> (Updated Feb. 1, 2017, 10:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Using `.Times(1)` with `EXPECT_CALL` is redundant, as this
> is implied by default. This patch removes all such remaining
> occurrences from the Mesos tests.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 7d728547da9fae0e169890add06fc9c2b26a447f 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> ba268b6918be0e7226a975c38eff5620b076083f 
>   src/tests/containerizer/cpu_isolator_tests.cpp 
> d61a8dc579ca95ae5a926ceab80f8efe4098bf91 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> d5332abd6f7bd9b3fce635d3db9202b6a56a1586 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> c27335731924509632ec96cc01a4b4415f108a30 
>   src/tests/containerizer/memory_isolator_tests.cpp 
> bcf8e8920ed5e1e1e3d6a3c4f4e33fd5451fb87a 
>   src/tests/containerizer/posix_rlimits_isolator_tests.cpp 
> fc57e237cf13d7423614d43bee9d43c783732e7c 
>   src/tests/exception_tests.cpp 7b66bd1b0beb004641fe0a08e02db168f2653354 
>   src/tests/fetcher_cache_tests.cpp 7dfc822ee4e1c9fe432b749bacf08e07ee50abf1 
>   src/tests/gc_tests.cpp d9776b602bee780b9732cd93d06b9c8f3cc8a4d0 
>   src/tests/master_authorization_tests.cpp 
> f973cf49cb07d2acc762997003309327ae49992a 
>   src/tests/master_contender_detector_tests.cpp 
> d41caeae19919ad1ff0693d59b64d8a6b385678d 
>   src/tests/master_maintenance_tests.cpp 
> 9861d1186e046a4fd547e73825c02b71eedd5ce2 
>   src/tests/master_tests.cpp da7094dbbafbb0ab1153a0a4a6fcabd63888d67a 
>   src/tests/rate_limiting_tests.cpp f105b67cd1f52ece6dcf8ca849f11b63e3d7dfa0 
>   src/tests/reconciliation_tests.cpp fc0ff9137c13f839731d8f228423b05f386463b5 
>   src/tests/registrar_zookeeper_tests.cpp 
> 598a93059b062438f0bebb5273e6965604db39fc 
>   src/tests/resource_offers_tests.cpp 
> 72ceb8696a7277f9add6d89adb55aa08665bdeb5 
>   src/tests/slave_recovery_tests.cpp 9cf1d461cf5c6ac1f51ac963360a17e37f558505 
>   src/tests/slave_tests.cpp 7f357a653335b559b5c78d4618926715dc4a63c7 
>   src/tests/status_update_manager_tests.cpp 
> 2d2f1ec82559dab9b8f7bf043cf23f38b7423fcb 
> 
> Diff: https://reviews.apache.org/r/56187/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56139: Removed redundant 'Times(1)' from master validation tests.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Feb. 1, 2017, 1 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56139/
> ---
> 
> (Updated Feb. 1, 2017, 1 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes calls to `.Times(1)` from `EXPECT_CALL`
> invocations in the master validation tests because it is
> redundant.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> edb57407e08cdbd8fbf10a9e1493cab3b4979bb8 
> 
> Diff: https://reviews.apache.org/r/56139/diff/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56052: Added the 'Secret' protobuf message.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On Jan. 31, 2017, 11:44 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56052/
> ---
> 
> (Updated Jan. 31, 2017, 11:44 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6996
> https://issues.apache.org/jira/browse/MESOS-6996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new `Secret` protobuf message which
> is designed to serve as a generic mechanism for passing
> priviledged information within Mesos.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593 
>   include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0 
> 
> Diff: https://reviews.apache.org/r/56052/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56055: Added validation for 'LAUNCH_NESTED_CONTAINER_SESSION'.

[Jan Schlicht]

> On Jan. 30, 2017, 10:24 a.m., Jan Schlicht wrote:
> > src/tests/slave_validation_tests.cpp, line 282
> > 
> >
> > Indent with 4 spaces.
> 
> Greg Mann wrote:
> Unfortunately, I think our style guide is ambiguous on this point? And it 
> seems that within the Mesos codebase we see about equal numbers of both 
> cases, indenting 2 and indenting 4 spaces after a linebreak like this. A 
> little playing around with `clang-format` seems to suggest that it prefers 
> indenting 2 spaces in this case. I don't have a strong opinion, happy to do 
> whatever is the "right" thing here. Local consistency is probably the most 
> important, and since all such occurrences in this file are part of the new 
> tests added in this review chain, I can set them all to be either 2 or 4.
> 
> Greg Mann wrote:
> I also changed the newline pattern for one of these occurrences to 
> something I prefer a bit more (no opinion on 2 vs. 4 spaces in the indent, 
> I'm fine with either):
> ```
> Environment::Variable* variable = launch
>   ->mutable_command()
>   ->mutable_environment()
>   ->mutable_variables()
>   ->Add();
> ```
> 
> Greg Mann wrote:
> I'm gonna drop this issue for now; if there's a case to be made for 4 
> spaces, I'm happy to change it.

I also don't have an opinion on 2 vs. 4 spaces. Just wanted it to be consistent 
with https://reviews.apache.org/r/55955 where similar occurrences are indented 
with 4 spaces.


- Jan


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


On Jan. 31, 2017, 11:19 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> ---
> 
> (Updated Jan. 31, 2017, 11:19 p.m.)
> 
> 
> Review request for mesos, Jan Schlicht and Vinod Kone.
> 
> 
> Bugs: MESOS-6887
> https://issues.apache.org/jira/browse/MESOS-6887
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_validation_tests.cpp 
> 5de771114982751e7796f55dcacd4384c6989efb 
> 
> Diff: https://reviews.apache.org/r/56055/diff/
> 
> 
> Testing
> ---
> 
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 56217: Added support for general checks to default executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56218: Added some check tests for default executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 

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


Testing
---

Tested the complete chain starting at https://reviews.apache.org/r/56208/ 
make check


Thanks,

Alexander Rukletsov

Review Request 56216: Renamed health checker collection in default executor for clarity.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56213: Added check tests for command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/tests/check_tests.cpp b5a5d8e80c80b480992a3c8160ee7d0e4443111c 
  src/tests/mesos.hpp 71e919702bccd31bf1e14205cd1e30d473883d48 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56212: Added support for general checks to command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56215: Reused previous task status to generate a new one in default executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56214: Hashed unacknowledged updates by UUID string in default executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/default_executor.cpp 97eee05cac8cb1f62d43e2aecc08a8e54e49eac3 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56211: Renamed health checker in command executor for clarity.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56210: Reused previous task status to generate a new one in command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

When a new task status update is generated in the executor, we have
to make sure specific data is duplicated from the previous update
to, e.g., avoid shadowing of those data during reconciliation. For
instance, consider a check status being sent; in this status update
we must include the latest known health information.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56208: Updated checks library with general check support.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

Add support for general checks, i.e. defined by CheckInfo, in
checking library. A general check can be either an command or
an HTTP request. The library performs the requested check at
the specified interval and sends the result to the framework
via a task status update. If the current result is the same as
the previous result, no status update is sent.


Diffs
-

  src/checks/checker.hpp dc293f3d3613dec716510d269829f8a6f406c277 
  src/checks/checker.cpp 8716e4cc684e6c4b6b76d8ca53221be06d10b2a6 
  src/checks/health_checker.hpp 95da1ff7dd6b222a93076633eb3757ec9aa43cf6 
  src/checks/health_checker.cpp 58380dc18896f659aa9c4fb4bb567a55bba97f6b 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Review Request 56209: Hashed unacknowledged updates by UUID string in command executor.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 

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


Testing
---

See https://reviews.apache.org/r/56218/


Thanks,

Alexander Rukletsov

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [53790, 54712, 54878, 53791]

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

- Mesos Reviewbot


On Feb. 1, 2017, 11:51 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Feb. 1, 2017, 11:51 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp 9648638c1d4b1f1c49708e058e9b930c5ae32138 
>   src/tests/containerizer/rootfs.cpp f11f126fa4f3130b6778bfe7e7d0f025a638f06e 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 56144: Added unit test for http::Headers abstraction.

[Gilbert Song]

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

(Updated Feb. 2, 2017, 12:40 a.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
Lin, and Timothy Chen.


Changes
---

Added Headers::empty() test.


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


Repository: mesos


Description
---

Added unit test for http::Headers abstraction.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/http_tests.cpp 
b12849e506f4340b865840072e583dfddecaf02a 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54537: Support 'Basic' auth docker registry on Unified Containerizer.

[Gilbert Song]

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

(Updated Feb. 2, 2017, 12:39 a.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
Lin, and Timothy Chen.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

This patch implements the support for 'Basic' docker registry
authorization. It is tested by a local authenticated private
registry using 'localhost:443/alpine' docker image.
Please note that the AWS ECS uses Basic authorization but it
does not work yet due to the redirect issue MESOS-5172.


Diffs (updated)
-

  src/uri/fetchers/docker.cpp 5dd7b91a5302067ce150bd632a05eccaf424a8a8 

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


Testing
---

make check

Tested with local authenticated registry. Please follow the steps below:

1. Start a local private registry and push an image to it.
```
docker run -d -p 443:5000 --restart=always --name registry \
  -v `pwd`/auth:/auth \
  -e "REGISTRY_AUTH=htpasswd" \
  -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
  -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
  -v `pwd`/certs:/certs \
  -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
  -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
  registry:2
```
(*Note: need to generate TLS certificate file and key first)

Then, push an image to the registry.
```
docker push localhost:443/alpine
```

2. Use `mesos-execute` to test the `localhost:443/alpine` image.
(*Note: need to configure the curl using the curl's default RC file), e.g., in 
`~/.curlrc` file:
cacert = "/path/to/cacert.pem"


Thanks,

Gilbert Song

Re: Review Request 56116: Implemented new http::Headers abstraction for WWW-Authenticate.

[Gilbert Song]

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

(Updated Feb. 2, 2017, 12:39 a.m.)


Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
Lin, and Timothy Chen.


Changes
---

Added Headers::emtpy().


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


Repository: mesos


Description
---

This patch introduced a new abstraction for http::Headers and helper
methods for the http::Headers hashmap, as well as a new
http::WWWAuthenticate class for the new `Headers` abstraction.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
4b11e085c3abe8d656b0e358b1d3e7bdccae9177 
  3rdparty/libprocess/src/http.cpp 4fd62c8372149a426eb4a70fbc28e423c5962820 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 54537: Support 'Basic' auth docker registry on Unified Containerizer.

[Gilbert Song]

> On Feb. 1, 2017, 8:40 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, line 801
> > 
> >
> > s/type/scheme/

I would go with "Unsupported auth-scheme:", which might be more accurate.


> On Feb. 1, 2017, 8:40 p.m., Jie Yu wrote:
> > src/uri/fetchers/docker.cpp, line 579
> > 
> >
> > Update this? can you do a sweep to fix all the comments?

I should have swept them carefully. :(


- Gilbert


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


On Feb. 1, 2017, 5:48 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54537/
> ---
> 
> (Updated Feb. 1, 2017, 5:48 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, Shuai 
> Lin, and Timothy Chen.
> 
> 
> Bugs: MESOS-6758
> https://issues.apache.org/jira/browse/MESOS-6758
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch implements the support for 'Basic' docker registry
> authorization. It is tested by a local authenticated private
> registry using 'localhost:443/alpine' docker image.
> Please note that the AWS ECS uses Basic authorization but it
> does not work yet due to the redirect issue MESOS-5172.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 5dd7b91a5302067ce150bd632a05eccaf424a8a8 
> 
> Diff: https://reviews.apache.org/r/54537/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with local authenticated registry. Please follow the steps below:
> 
> 1. Start a local private registry and push an image to it.
> ```
> docker run -d -p 443:5000 --restart=always --name registry \
>   -v `pwd`/auth:/auth \
>   -e "REGISTRY_AUTH=htpasswd" \
>   -e "REGISTRY_AUTH_HTPASSWD_REALM=Registry Realm" \
>   -e REGISTRY_AUTH_HTPASSWD_PATH=/auth/htpasswd \
>   -v `pwd`/certs:/certs \
>   -e REGISTRY_HTTP_TLS_CERTIFICATE=/certs/localhost.crt \
>   -e REGISTRY_HTTP_TLS_KEY=/certs/localhost.key \
>   registry:2
> ```
> (*Note: need to generate TLS certificate file and key first)
> 
> Then, push an image to the registry.
> ```
> docker push localhost:443/alpine
> ```
> 
> 2. Use `mesos-execute` to test the `localhost:443/alpine` image.
> (*Note: need to configure the curl using the curl's default RC file), e.g., 
> in `~/.curlrc` file:
> cacert = "/path/to/cacert.pem"
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>