Re: Review Request 52379: Added agent flag '--oci_store_dir'.

[Qian Zhang]

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

(Updated Nov. 25, 2016, 3:48 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added agent flag '--oci_store_dir'.


Diffs (updated)
-

  docs/configuration.md efe3e9bd9d203a7ba44adf4ead24f14b8b577637 
  docs/endpoints/slave/state.json.md 0f82c1926404e79b281b2ea5f4d0ca21323aeded 
  docs/endpoints/slave/state.md b34459e8624f0b29e927ff79be7fc845ac88080b 
  src/slave/flags.hpp 3c292bac9394347318865f49782907def6541742 
  src/slave/flags.cpp 76609f94482a792f4c8db480e604d98d172abb04 
  src/slave/http.cpp aa9f492fc74283b5a6f24a8b7189f1ecbe41488f 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52382: Added stubs for OCI store.

[Qian Zhang]

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

(Updated Nov. 25, 2016, 3:48 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added stubs for OCI store.


Diffs (updated)
-

  src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
  src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
  src/slave/containerizer/mesos/provisioner/oci/store.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/store.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 52349: Add protobuf messages for OCI image spec.

[Qian Zhang]

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

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


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add protobuf messages for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  include/mesos/oci/spec.proto PRE-CREATION 
  src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 53948: Cleaned up includes test/utils.cpp.

[Benjamin Bannier]

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

(Updated Nov. 25, 2016, 8:43 a.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Cleanup patch deps.


Repository: mesos


Description
---

Cleaned up includes test/utils.cpp.


Diffs
-

  src/tests/utils.hpp 140ebaaae43b03568ec49891635f0660cdfb4c85 
  src/tests/utils.cpp eb36616f68d81d33d4bd04a7f23295e8c7558fc8 

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


Testing
---

Tested as part of https://reviews.apache.org/r/53950/.


Thanks,

Benjamin Bannier

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Guangya Liu]

> On 十一月 25, 2016, 3:20 a.m., Guangya Liu wrote:
> > src/common/roles.cpp, lines 110-130
> > 
> >
> > Can we simplify the logic as your diagram above:
> > 
> > ```
> > if (frameworkInfo.roles_size() > 0 &&
> > frameworkInfo.has_role()) {
> > 
> > }
> > 
> > if (frameworkHasCapability(
> > frameworkInfo,
> > FrameworkInfo::Capability::MULTI_ROLE)) {
> >   if (frameworkInfo.roles_size() == 0 &&
> >   frameworkInfo.has_role()) {
> >   
> >   }
> > } else {
> >   if (frameworkInfo.roles_size() > 0 &&
> >   !frameworkInfo.has_role()) {
> >   
> >   }
> > }
> > ```
> 
> Jay Guo wrote:
> I don't see this simplifies the logic, since inevitably we need to have 
> three error cases. And since the diagram describes the results in 2 
> categories, w/o MULTI_ROLE, I think current implementation may reflect that 
> pattern better, what do you think?

I want to use less nested `if-else`. ;)

```
  } else {
if (frameworkInfo.roles_size()) {
  if (frameworkInfo.has_role()) {
return Error("Only one of FrameworkInfo.role and FrameworkInfo.roles "
 "must be set at a time.");
  } else {
return Error("If FrameworkInfo.roles is set, then the MULTI_ROLE "
 "framework capability must be provided.");
  }
}   
  }

```


> On 十一月 25, 2016, 3:20 a.m., Guangya Liu wrote:
> > src/common/roles.cpp, line 138
> > 
> >
> > I think that you do not need this as there is no need to persist roles 
> > in this validation.
> 
> Jay Guo wrote:
> This is used to check duplicate entries in roles. I added some comments 
> in the revision.

I see, we need check here.


- Guangya


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


On 十一月 25, 2016, 6:45 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated 十一月 25, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Guangya Liu and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> Two test cases are added, one is for validateRoles method and another
> ensures that the master rejects subscription when provided invalid
> roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Jay Guo]

> On Nov. 25, 2016, 5:24 a.m., Qian Zhang wrote:
> > src/common/roles.cpp, lines 141-143
> > 
> >
> > Can you check the duplicate entries by referring the code below? That 
> > seems a more clear way to me.
> > 
> > https://github.com/apache/mesos/blob/1.1.0/src/slave/containerizer/containerizer.cpp#L191:L197

We also check indiviual `role` in the `roles` to not contain illegal 
characters. So I put them into same loop. I updated the patch with some more 
comments. Let's me know what you think.


- Jay


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


On Nov. 25, 2016, 6:45 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 25, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Guangya Liu and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> Two test cases are added, one is for validateRoles method and another
> ensures that the master rejects subscription when provided invalid
> roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Jay Guo]

> On Nov. 25, 2016, 5:17 a.m., Qian Zhang wrote:
> > src/common/roles.cpp, lines 112-118
> > 
> >
> > What if the `MULTI_ROLE` capability is provided, but 
> > `frameworkInfo.roles` is empty? So in this case framework will be 
> > considered to subscribe to `"*"` role?
> > 
> > And based on the description in MESOS-6629, only one of 
> > FrameworkInfo.role and FrameworkInfo.roles **MUST** be set at a time. But 
> > it seems in your code here, both FrameworkInfo.role and FrameworkInfo.roles 
> > can be unset at a time and can pass the validation in this method, however 
> > I think that should be an invalid case.

I think if `MULTI_ROLE` is provided, but none of `role` and `roles` is set, the 
framework should be considered to subscribe with `"*"`. It similiar to single 
role framework subscribing with empty `role`. Maybe we need some inputs from 
BenB...


- Jay


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


On Nov. 25, 2016, 6:45 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 25, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Guangya Liu and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> Two test cases are added, one is for validateRoles method and another
> ensures that the master rejects subscription when provided invalid
> roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Jay Guo]

> On Nov. 25, 2016, 3:20 a.m., Guangya Liu wrote:
> > src/common/roles.cpp, lines 110-130
> > 
> >
> > Can we simplify the logic as your diagram above:
> > 
> > ```
> > if (frameworkInfo.roles_size() > 0 &&
> > frameworkInfo.has_role()) {
> > 
> > }
> > 
> > if (frameworkHasCapability(
> > frameworkInfo,
> > FrameworkInfo::Capability::MULTI_ROLE)) {
> >   if (frameworkInfo.roles_size() == 0 &&
> >   frameworkInfo.has_role()) {
> >   
> >   }
> > } else {
> >   if (frameworkInfo.roles_size() > 0 &&
> >   !frameworkInfo.has_role()) {
> >   
> >   }
> > }
> > ```

I don't see this simplifies the logic, since inevitably we need to have three 
error cases. And since the diagram describes the results in 2 categories, w/o 
MULTI_ROLE, I think current implementation may reflect that pattern better, 
what do you think?


> On Nov. 25, 2016, 3:20 a.m., Guangya Liu wrote:
> > src/common/roles.cpp, line 138
> > 
> >
> > I think that you do not need this as there is no need to persist roles 
> > in this validation.

This is used to check duplicate entries in roles. I added some comments in the 
revision.


> On Nov. 25, 2016, 3:20 a.m., Guangya Liu wrote:
> > src/common/roles.cpp, line 150
> > 
> >
> > kill this as well

see above comments


- Jay


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


On Nov. 25, 2016, 6:45 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 25, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Guangya Liu and Qiang Zhang.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We need to do necessary validation for the conflicts of role, roles
> and MULTI_ROLE capability. It complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> Two test cases are added, one is for validateRoles method and another
> ensures that the master rejects subscription when provided invalid
> roles.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/master/validation.hpp b8389460f34b3531f2b6ff93f18f496c01e1a079 
>   src/master/validation.cpp 42d9b4a8784c2a161b74d7b46619cc22272e14e3 
>   src/tests/master_validation_tests.cpp 
> f893067859425967654401f3226149268b51cf57 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Qian Zhang]

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




src/common/roles.cpp (lines 141 - 143)


Can you check the duplicate entries by referring the code below? That seems 
a more clear way to me.

https://github.com/apache/mesos/blob/1.1.0/src/slave/containerizer/containerizer.cpp#L191:L197


- Qian Zhang


On Nov. 24, 2016, 11:30 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 24, 2016, 11:30 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For backwards compatibility, we keep role in FrameworkInfo and do
> necessary validation, which complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> 
> Diffs
> -
> 
>   include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Qian Zhang]

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




src/common/roles.cpp (lines 112 - 118)


What if the `MULTI_ROLE` capability is provided, but `frameworkInfo.roles` 
is empty? So in this case framework will be considered to subscribe to `"*"` 
role?

And based on the description in MESOS-6629, only one of FrameworkInfo.role 
and FrameworkInfo.roles **MUST** be set at a time. But it seems in your code 
here, both FrameworkInfo.role and FrameworkInfo.roles can be unset at a time 
and can pass the validation in this method, however I think that should be an 
invalid case.



src/common/roles.cpp (line 119)


Kill this line.


- Qian Zhang


On Nov. 24, 2016, 11:30 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 24, 2016, 11:30 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For backwards compatibility, we keep role in FrameworkInfo and do
> necessary validation, which complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> 
> Diffs
> -
> 
>   include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 54068: Fixed some nits in hooks interfaces.

[Till Toenshoff]

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

Review request for mesos, Adam B, Gastón Kleiman, and Kapil Arya.


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


Repository: mesos


Description
---

see summary


Diffs
-

  include/mesos/hook.hpp f0606e3a68fa179cf7ea036f10563ef47c2aefa7 
  src/examples/test_hook_module.cpp 5e91a71f2450cf3c37eb9039ef28c026095c917e 
  src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
  src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 

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


Testing
---

make check && functional testing of entire RR chain.


Thanks,

Till Toenshoff

Re: Review Request 54038: Added new hook for covering executor and task environment.

[Till Toenshoff]

> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > include/mesos/hook.hpp, line 111
> > 
> >
> > "Deprecated"? Will we ever remove this hook?

I was discussing this with Kapil and we need to address this very soon. 
Definitely a tech-debt. Created a ticket for tracking that job - 
https://issues.apache.org/jira/browse/MESOS-6641


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > include/mesos/hook.hpp, lines 146-147
> > 
> >
> > Is `sandboxDirectory` the host or container path? Absolute or relative?
> > Same question for `mappedDirectory`, also what is it? sandbox container 
> > path?

Super confusing indeed.

For a user, the sandbox-directory is the absolute, mapped path within the 
container. See 
https://github.com/apache/mesos/blob/8fd64fd5efb898baf99a74ea9104205969d9807b/src/slave/flags.cpp#L629-L633

For the superceded docker hooks, the sandbox-directory is the absolute 
container work directory roof fs path on the agent host. See the callup of the 
old variant of this hook 
https://github.com/apache/mesos/blob/8fd64fd5efb898baf99a74ea9104205969d9807b/src/slave/containerizer/docker.cpp#L1135-L1136
 as you noted below.

Will rename these towards `containerWorkDirectory, mappedSandboxDirectory` and 
also in a follow-up patch on the deprecated implementations.


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > include/mesos/module/hook.proto, line 28
> > 
> >
> > Why add this TODO instead of filing a JIRA? We can always add new 
> > protobuf fields later, when we're ready.

True - filed a JIRA - https://issues.apache.org/jira/browse/MESOS-6642


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/hook/manager.cpp, line 221
> > 
> >
> > Update this comment (and others) if you update the proto message name

Thanks for the reminder :) - appreciated!


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/slave/containerizer/docker.cpp, line 1132
> > 
> >
> > "as soon as possible" isn't very specific. How about "... remove this 
> > entire block after Mesos 1.2's deprecation cycle ends."

Yup.


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/slave/containerizer/docker.cpp, lines 1193-1195
> > 
> >
> > Sounds like a great thing to Warn about in the agent log. Planning on 
> > doing this in a subsequent patch?

Yes - however its a bit more involved and hence I am deferring that right now - 
hence the TODO - also created a JIRA issue - 
https://issues.apache.org/jira/browse/MESOS-6643


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/slave/containerizer/docker.cpp, lines 1224-1225
> > 
> >
> > So, if executorEnvironment and taskEnvironment both have the same keys, 
> > then the taskEnvironment will override?

Yes - will add a comment explaining this.


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > include/mesos/hook.hpp, line 128
> > 
> >
> > Or TASK_ERROR? I thought FAILED meant that it ran and returned a 
> > non-zero exit code.

No, the stated should be correct. When the hook returns a failed future, the 
(docker) containerizer returns a failed future for `launch` which then gets 
processed at 
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L4566, turning 
the task state into `TASK_FAILED`.
That is what currently happens - now the question might be if this is really 
what we want and that might be worth a discussion.


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/hook/manager.cpp, lines 245-249
> > 
> >
> > Could save a couple lines if you invert the if statement and push the 
> > MergeFrom into it.
> > ```
> > if (result.isSome()) {
> >   info.MergeFrom(result.get());
> > }
> > ```

Doh :)


> On Nov. 24, 2016, 3:38 a.m., Adam B wrote:
> > src/slave/containerizer/docker.cpp, lines 1178-1179
> > 
> >
> > I still don't understand why `sandboxDirectory = container->directory` 
> > and `mappedDirectory = flags.sandbox_directory`. Seems backwards as far as 
> > "sandbox directory" goes.

s.o.


- Till


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


On Nov. 25, 2016, 4:10 a.m., 

Re: Review Request 54038: Added new hook for covering executor and task environment.

[Till Toenshoff]

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

(Updated Nov. 25, 2016, 4:10 a.m.)


Review request for mesos, Adam B, Gastón Kleiman, Kapil Arya, and Joseph Wu.


Changes
---

Addressed comments - thanks for the reviews!


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


Repository: mesos


Description
---

For being able to supply environment additions for both, a task and
its executor separately we need to introduce a new hook as the
existing ones slavePreLaunchDockerHook (deprecated) as well as
slavePreLaunchDockerEnvironmentDecorator do not allow for this.

This new hook will likely allow for further additions like e.g.
adding volumes without having to adapt the signature but only the
returned proto message TaskExecutorDecoratorInfo.


Diffs (updated)
-

  include/mesos/hook.hpp f0606e3a68fa179cf7ea036f10563ef47c2aefa7 
  include/mesos/module/hook.proto PRE-CREATION 
  src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
  src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
  src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 
  src/slave/containerizer/docker.cpp ccabf99f305d7874e1c46bc618ea74341eb281ef 

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


Testing
---

make check

*WIP - functional test pending - unit tests pending - WIP*


Thanks,

Till Toenshoff

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Guangya Liu]

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




include/mesos/roles.hpp (line 87)


I think it may not good to do the validatioin here as the validation has 
some logic related to framework capability and not pure role validation, you 
are actually validating frameworkinfo here.

What about moving this to master/validation.cpp by adding a new namespace 
of `framework` for validation?



src/common/roles.cpp (lines 110 - 130)


Can we simplify the logic as your diagram above:

```
if (frameworkInfo.roles_size() > 0 &&
frameworkInfo.has_role()) {

}

if (frameworkHasCapability(
frameworkInfo,
FrameworkInfo::Capability::MULTI_ROLE)) {
  if (frameworkInfo.roles_size() == 0 &&
  frameworkInfo.has_role()) {
  
  }
} else {
  if (frameworkInfo.roles_size() > 0 &&
  !frameworkInfo.has_role()) {
  
  }
}
```



src/common/roles.cpp (lines 116 - 117)


We prefer putting the blank space at the beginning.

```
return Error("FrameworkInfo.role should NOT be set when MULTI_ROLE"
 " framework capability is provided.");
```

Ditto for other messages.



src/common/roles.cpp (line 121)


We do not want to use `.roles_size()` as boolean, but should be `if 
(frameworkInfo.roles_size() > 0) {`.



src/common/roles.cpp (line 124)


s/must/can



src/common/roles.cpp (line 138)


I think that you do not need this as there is no need to persist roles in 
this validation.



src/common/roles.cpp (line 150)


kill this as well



src/master/master.cpp (lines 2330 - 2335)


Can you please also add a new test case to test the register failed with 
mutiple roles in roles_tests.cpp like 
https://github.com/apache/mesos/blob/master/src/tests/master_tests.cpp#L4892



src/tests/role_tests.cpp (lines 666 - 717)


You may want to put this to master_validation_test.cpp if above comments 
addressed.



src/tests/role_tests.cpp (line 706)


period to the end.



src/tests/role_tests.cpp (line 710)


period to the end.


- Guangya Liu


On Nov. 24, 2016, 3:30 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 24, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For backwards compatibility, we keep role in FrameworkInfo and do
> necessary validation, which complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> 
> Diffs
> -
> 
>   include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54061: Augmented FrameworkInfo to include repeated roles.

[Guangya Liu]

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



Can you update the summary as `Augmented FrameworkInfo to support multiple 
roles.`? As you are not only adding repeated roles but also a framework 
capability.

- Guangya Liu


On 十一月 24, 2016, 3:29 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54061/
> ---
> 
> (Updated 十一月 24, 2016, 3:29 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6628
> https://issues.apache.org/jira/browse/MESOS-6628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the initial patch to introduce multi-role frameworks.
> A new section `repeated string roles` is added to FrameworkInfo.
> Old `optional string role` is kept for backwards compatability.
> To distinguish single-role and multi-role frameworks, a new framework
> capability `MULTI_ROLE` is introduced. See MESOS-6628 for more info.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
>   include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 
> 
> Diff: https://reviews.apache.org/r/54061/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54067: Redirected mesos containerizer 'local' flag through IOSwitchboard.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Nov. 24, 2016, 5:06 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54067/
> ---
> 
> (Updated Nov. 24, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6467
> https://issues.apache.org/jira/browse/MESOS-6467
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Redirected mesos containerizer 'local' flag through IOSwitchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> c8d43f918981d06a73662cf1be61081915816ca5 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d9e6bb4e4d62e6aa542c0bc7e9a5e0c6dcc670e5 
>   src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> c369b313b5ed375cc8790c4f6e5cc22f6f9095c1 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> fa773c8badf4ddca11b5dfe8033b5ffb16dba213 
> 
> Diff: https://reviews.apache.org/r/54067/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check
> sudo src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54061: Augmented FrameworkInfo to include repeated roles.

[Qian Zhang]

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




include/mesos/mesos.proto (line 334)


There is a redundant space between "and" and "move".



include/mesos/v1/mesos.proto (line 334)


Ditto


- Qian Zhang


On Nov. 24, 2016, 11:29 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54061/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6628
> https://issues.apache.org/jira/browse/MESOS-6628
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the initial patch to introduce multi-role frameworks.
> A new section `repeated string roles` is added to FrameworkInfo.
> Old `optional string role` is kept for backwards compatability.
> To distinguish single-role and multi-role frameworks, a new framework
> capability `MULTI_ROLE` is introduced. See MESOS-6628 for more info.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
>   include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 
> 
> Diff: https://reviews.apache.org/r/54061/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 47088: Dropped http status check in HealthCheckTest.HealthStatusChange.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On May 7, 2016, 9:07 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47088/
> ---
> 
> (Updated May 7, 2016, 9:07 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> Artem Harutyunyan, Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-1802
> https://issues.apache.org/jira/browse/MESOS-1802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Dropped http status check because so far we could not guarantee the
> http queries order match the `statusUpdate` order.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
> 
> Diff: https://reviews.apache.org/r/47088/diff/
> 
> 
> Testing
> ---
> 
> Using
> 
> ```
> GLOG_v=1 ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.HealthStatusChange" --verbose 
> --gtest_break_on_failure --gtest_repeat=-1
> ```
> 
> More than 1000 interactions in my slow vm.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46307: Ignored subsequent status update in HealthStatusChange tests.

[Alexander Rukletsov]

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




src/tests/health_check_tests.cpp (line 779)


Since you're touching this test, let's mark it `const` and rename to 
`healthCheckCmd`.


- Alexander Rukletsov


On Nov. 24, 2016, 10:51 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46307/
> ---
> 
> (Updated Nov. 24, 2016, 10:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-1802
> https://issues.apache.org/jira/browse/MESOS-1802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In HealthStatusChange test cases, we launch a task that toggles between
> healthy and unhealthy, and will never be killed because no consecutive
> health failures occur. We need to ignore subsequent status updates: it
> is possible to continue to receive status updates before we stop the
> driver.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
> 
> Diff: https://reviews.apache.org/r/46307/diff/
> 
> 
> Testing
> ---
> 
> # I still could not reproduce the problem in old code after repeatedly tests. 
> So seems no way to verify whether my assumption is correct or not.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 46307: Ignored subsequent status update in HealthStatusChange tests.

[haosdent huang]

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

(Updated Nov. 24, 2016, 10:51 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, Neil 
Conway, and Timothy Chen.


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


Repository: mesos


Description (updated)
---

In HealthStatusChange test cases, we launch a task that toggles between
healthy and unhealthy, and will never be killed because no consecutive
health failures occur. We need to ignore subsequent status updates: it
is possible to continue to receive status updates before we stop the
driver.


Diffs
-

  src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 

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


Testing
---

# I still could not reproduce the problem in old code after repeatedly tests. 
So seems no way to verify whether my assumption is correct or not.


Thanks,

haosdent huang

Re: Review Request 46307: Ignored subsequent status update in HealthStatusChange tests.

[Alexander Rukletsov]

> On April 19, 2016, 2:35 p.m., Neil Conway wrote:
> > This patch does not solve the flakiness for me: failed once after 2 
> > iterations, then again after 77 iterations. Verbose test log here: 
> > https://gist.github.com/neilconway/e6134b4717ee022e7fc32a1f95619fa9
> 
> haosdent huang wrote:
> Thank you very much for your test! I saw you use `vagrant@archlinux`, may 
> you share your vagrantfile to me? So that I could try to reproduce in my 
> local.
> 
> haosdent huang wrote:
> ```
> I0420 00:33:13.497138 15400 http.cpp:313] HTTP GET for /master/state from 
> 10.0.2.15:44478
> Received task health update, healthy: true
> I0420 00:33:13.502598 15400 slave.cpp:3201] Handling status update 
> TASK_RUNNING (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for task 1 in 
> health state healthy of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765- 
> from executor(1)@10.0.2.15:37107
> I0420 00:33:13.504456 15400 status_update_manager.cpp:320] Received 
> status update TASK_RUNNING (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for 
> task 1 in health state healthy of framework 
> 7cf5923c-3d03-4ed6-826a-efa97f54e765-
> I0420 00:33:13.505009 15400 slave.cpp:3599] Forwarding the update 
> TASK_RUNNING (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for task 1 in 
> health state healthy of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765- 
> to master@10.0.2.15:41408
> I0420 00:33:13.505167 15400 slave.cpp:3509] Sending acknowledgement for 
> status update TASK_RUNNING (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for 
> task 1 in health state healthy of framework 
> 7cf5923c-3d03-4ed6-826a-efa97f54e765- to executor(1)@10.0.2.15:37107
> I0420 00:33:13.505524 15400 master.cpp:5069] Status update TASK_RUNNING 
> (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for task 1 in health state 
> healthy of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765- from agent 
> 7cf5923c-3d03-4ed6-826a-efa97f54e765-S0 at slave(76)@10.0.2.15:41408 
> (archlinux.vagrant.vm)
> I0420 00:33:13.505602 15400 master.cpp:5117] Forwarding status update 
> TASK_RUNNING (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) for task 1 in 
> health state healthy of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765-
> I0420 00:33:13.505738 15400 master.cpp:6725] Updating the state of task 1 
> of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765- (latest state: 
> TASK_RUNNING, status update state: TASK_RUNNING)
> I0420 00:33:13.505985 15400 master.cpp:4224] Processing ACKNOWLEDGE call 
> e19c76cc-096a-4398-b616-afb628b8e5b8 for task 1 of framework 
> 7cf5923c-3d03-4ed6-826a-efa97f54e765- (default) at 
> scheduler-5bd5e446-a017-45d9-8193-be7d23002487@10.0.2.15:41408 on agent 
> 7cf5923c-3d03-4ed6-826a-efa97f54e765-S0
> I0420 00:33:13.506142 15400 status_update_manager.cpp:392] Received 
> status update acknowledgement (UUID: e19c76cc-096a-4398-b616-afb628b8e5b8) 
> for task 1 of framework 7cf5923c-3d03-4ed6-826a-efa97f54e765-
> rm: cannot remove '/tmp/1NKfr1': No such file or directory
> I0420 00:33:13.508203 15400 http.cpp:178] HTTP GET for /slave(76)/state 
> from 10.0.2.15:44482
> ../../mesos/src/tests/health_check_tests.cpp:647: Failure
> Value of: (find).get()
>   Actual: 16-byte object <05-00 00-00 00-00 00-00 90-C4 2D-03 00-00 00-00>
> Expected: false
> Which is: false
> *** Aborted at 1461076393 (unix time) try "date -d @1461076393" if you 
> are using GNU date ***
> PC: @  0x1899ba0 testing::UnitTest::AddTestPartResult()
> *** SIGSEGV (@0x0) received by PID 15381 (TID 0x7f0aa958a7c0) from PID 0; 
> stack trace: ***
> 
> ```
> It looks like get `true` here. Let me try how to fix this.

There were at least two different issues in this test (see MESOS-1802), and 
this patch fixes just one. The one you see will be addressed in the next review 
in the chain.


- Alexander


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


On May 17, 2016, 4:46 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46307/
> ---
> 
> (Updated May 17, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-1802
> https://issues.apache.org/jira/browse/MESOS-1802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In HealthStatusChange test cases, we launch a task that toggles between
> healthy and unhealthy, and will never be killed because no consecutive
> health failures occur. We need to ignore subsequent status updates it
> is possible to continue to receive status updates 

Re: Review Request 46307: Ignored subsequent status update in HealthStatusChange tests.

[Alexander Rukletsov]

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


Ship it!




LGTM, modulo rebase.


src/tests/health_check_tests.cpp (line 561)


Looks like this has been fixed in another patch : ).



src/tests/health_check_tests.cpp (line 801)


But this is not!


- Alexander Rukletsov


On May 17, 2016, 4:46 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46307/
> ---
> 
> (Updated May 17, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Greg Mann, 
> Neil Conway, and Timothy Chen.
> 
> 
> Bugs: MESOS-1802
> https://issues.apache.org/jira/browse/MESOS-1802
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In HealthStatusChange test cases, we launch a task that toggles between
> healthy and unhealthy, and will never be killed because no consecutive
> health failures occur. We need to ignore subsequent status updates it
> is possible to continue to receive status updates before we stop the
> driver.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1c4a554ab07731963a4a38e3ae40b0323bf317bb 
> 
> Diff: https://reviews.apache.org/r/46307/diff/
> 
> 
> Testing
> ---
> 
> # I still could not reproduce the problem in old code after repeatedly tests. 
> So seems no way to verify whether my assumption is correct or not.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 54007: Printed complete health check configuration on task launch.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 1:11 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54007/
> ---
> 
> (Updated Nov. 23, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/54007/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52873: Cleaned up private members in HealthChecker class.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:52 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52873/
> ---
> 
> (Updated Nov. 23, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/52873/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52872: Used callback instead of `send()` for health status updates.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:52 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52872/
> ---
> 
> (Updated Nov. 23, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-6395
> https://issues.apache.org/jira/browse/MESOS-6395
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since HealthChecker is now used as a library only and does not live
> in a separate OS process, there is no need to use libprocess message
> sending for health status updates; a callback will do.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
>   src/launcher/default_executor.cpp f4e1ea4816fee9a2f6d59bb534933b66013f11cf 
>   src/launcher/executor.cpp ce0b199551447504bb95743df4ce9ec4a0443cd4 
> 
> Diff: https://reviews.apache.org/r/52872/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53986: Introduced process using decls to "tests/utils.cpp".

[Benjamin Bannier]

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

(Updated Nov. 24, 2016, 5:40 p.m.)


Review request for mesos and Alexander Rukletsov.


Summary (updated)
-

Introduced process using decls to "tests/utils.cpp".


Repository: mesos


Description
---

Introduced process using decls to test/utils.cpp.


Diffs
-

  src/tests/utils.cpp eb36616f68d81d33d4bd04a7f23295e8c7558fc8 

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


Testing
---

Tested as part of https://reviews.apache.org/r/53950/.


Thanks,

Benjamin Bannier

Re: Review Request 53950: Avoided hardcoding ports in some tests.

[Benjamin Bannier]

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

(Updated Nov. 24, 2016, 6:39 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


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


Repository: mesos


Description
---

Avoided hardcoding ports in some tests.


Diffs
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
c478d56ffd734b26fd2dbd7bb1ca02ce929f3f16 
  src/tests/health_check_tests.cpp 2c7022a1210ade6d7eb594c4f8934f2bdb2aee35 

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


Testing (updated)
---

* `make check` (SSL build OS X clang trunk w/ optimizations; default build 
ubuntu-14.04 gcc-4.8.4 w/o optimizations)
* tested with in-tree parallel test runner: `GTEST_FILTER='HealthCheckTest.*'  
~/src/mesos/support/mesos-gtest-runner.py ./mesos-tests` which would fail about 
for about 50% before this fix, and not measurably anymore after.


Thanks,

Benjamin Bannier

Re: Review Request 52871: Ensured default executor ignores health updates for terminated tasks.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 1:06 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52871/
> ---
> 
> (Updated Nov. 23, 2016, 1:06 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> After the task has been terminated, its health updates become
> irrelevant and should be ignored. Also if the default executor
> shuts down, we can safely stop all health checkers.
> 
> Technically health checking should be stopped right before
> TASK_KILLING update is sent to avoid subsequent TASK_RUNNING
> updates, but the default executor currently does not support
> TASK_KILLING.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp f4e1ea4816fee9a2f6d59bb534933b66013f11cf 
> 
> Diff: https://reviews.apache.org/r/52871/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53949: Added test helper to obtain unused port.

[Alexander Rukletsov]

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




src/tests/utils.hpp (line 77)


Capitalize "while".



src/tests/utils.hpp (line 80)


#include 



src/tests/utils.cpp (lines 73 - 74)


We definitely need to say that our assumption is that socket implementation 
closes the socket on destruction.


- Alexander Rukletsov


On Nov. 22, 2016, 3:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53949/
> ---
> 
> (Updated Nov. 22, 2016, 3:49 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-6618
> https://issues.apache.org/jira/browse/MESOS-6618
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test helper to obtain unused port.
> 
> 
> Diffs
> -
> 
>   src/tests/utils.hpp 140ebaaae43b03568ec49891635f0660cdfb4c85 
>   src/tests/utils.cpp eb36616f68d81d33d4bd04a7f23295e8c7558fc8 
> 
> Diff: https://reviews.apache.org/r/53949/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/53950/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52870: Ensured docker executor stops health checking terminated tasks.

[Gastón Kleiman]

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


Fix it, then Ship it!





src/docker/executor.cpp (line 367)


Same as the previous patch, we should probably stop the checker here.


- Gastón Kleiman


On Nov. 23, 2016, 12:48 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52870/
> ---
> 
> (Updated Nov. 23, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 
> 
> Diff: https://reviews.apache.org/r/52870/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52869: Ensured command executor stops health checking terminated tasks.

[Gastón Kleiman]

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




src/launcher/executor.cpp (line 624)


I just realized that we now have a check, so the additional checks wouldn't 
lead to an invalid transition.

But still, if we don't stop the health checker here, it will still be 
working, even though we'll throw away the results.


- Gastón Kleiman


On Nov. 23, 2016, 12:48 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52869/
> ---
> 
> (Updated Nov. 23, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp ce0b199551447504bb95743df4ce9ec4a0443cd4 
> 
> Diff: https://reviews.apache.org/r/52869/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52869: Ensured command executor stops health checking terminated tasks.

[Gastón Kleiman]

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




src/launcher/executor.cpp (line 624)


We should probably stop health checking here, in order to prevent 
`TASK_KILLING` -> `TASK_RUNNING` transitions.


- Gastón Kleiman


On Nov. 23, 2016, 12:48 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52869/
> ---
> 
> (Updated Nov. 23, 2016, 12:48 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp ce0b199551447504bb95743df4ce9ec4a0443cd4 
> 
> Diff: https://reviews.apache.org/r/52869/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52868: Health checks may be stopped on demand.

[Gastón Kleiman]

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



I think that `HealthChecker::stop()` should just terminate the actor, no need 
to introduce a boolean.

- Gastón Kleiman


On Nov. 23, 2016, 12:51 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52868/
> ---
> 
> (Updated Nov. 23, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Gastón Kleiman, and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/52868/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53986: Introduced process using decls to test/utils.cpp.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Nov. 22, 2016, 3:39 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53986/
> ---
> 
> (Updated Nov. 22, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced process using decls to test/utils.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/utils.cpp eb36616f68d81d33d4bd04a7f23295e8c7558fc8 
> 
> Diff: https://reviews.apache.org/r/53986/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/53950/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 53948: Cleaned up includes test/utils.cpp.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Nov. 22, 2016, 3:39 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53948/
> ---
> 
> (Updated Nov. 22, 2016, 3:39 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up includes test/utils.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/utils.hpp 140ebaaae43b03568ec49891635f0660cdfb4c85 
>   src/tests/utils.cpp eb36616f68d81d33d4bd04a7f23295e8c7558fc8 
> 
> Diff: https://reviews.apache.org/r/53948/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/53950/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54061, 54062]

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 Nov. 24, 2016, 3:30 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54062/
> ---
> 
> (Updated Nov. 24, 2016, 3:30 p.m.)
> 
> 
> Review request for mesos and Guangya Liu.
> 
> 
> Bugs: MESOS-6629
> https://issues.apache.org/jira/browse/MESOS-6629
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For backwards compatibility, we keep role in FrameworkInfo and do
> necessary validation, which complies with following matrix:
> 
> -- MULTI_ROLE is NOT set -
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  None   |
> |---|---|-|
> |No Role| Error |  None   |
> |---|---|-|
> 
> --- MULTI_ROLE is set 
> |---|-|
> |Roles  |No Roles |
> |---|---|-|
> |Role   | Error |  Error  |
> |---|---|-|
> |No Role| None  |  None   |
> |---|---|-|
> 
> 
> Diffs
> -
> 
>   include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
>   src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 
> 
> Diff: https://reviews.apache.org/r/54062/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53936: Added agent flags to enable/disable launching an io switchboard server.

[Kevin Klues]

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

(Updated Nov. 24, 2016, 5:08 p.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Rebased to update "Depends On" in chain.


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


Repository: mesos


Description
---

Added agent flags to enable/disable launching an io switchboard server.


Diffs (updated)
-

  src/slave/flags.hpp 3c292bac9394347318865f49782907def6541742 
  src/slave/flags.cpp 76609f94482a792f4c8db480e604d98d172abb04 

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


Testing
---

make


Thanks,

Kevin Klues

Re: Review Request 54067: Redirected mesos containerizer 'local' flag through IOSwitchboard.

[Kevin Klues]

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

(Updated Nov. 24, 2016, 5:06 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated patch summary and fixed test compilation (oops).


Summary (updated)
-

Redirected mesos containerizer 'local' flag through IOSwitchboard.


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


Repository: mesos


Description (updated)
---

Redirected mesos containerizer 'local' flag through IOSwitchboard.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
c8d43f918981d06a73662cf1be61081915816ca5 
  src/slave/containerizer/mesos/containerizer.cpp 
d9e6bb4e4d62e6aa542c0bc7e9a5e0c6dcc670e5 
  src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
  src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c369b313b5ed375cc8790c4f6e5cc22f6f9095c1 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
fa773c8badf4ddca11b5dfe8033b5ffb16dba213 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52867: Used `Duration::create()` for double -> Duration conversion.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:44 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52867/
> ---
> 
> (Updated Nov. 23, 2016, 12:44 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Gastón Kleiman, haosdent huang, and 
> Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Additionally persist health check parameters from the `HealthCheck`
> protobuf as class members to avoid code duplication.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/52867/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 54067: Redirected 'local' flag through IOSwitchboard.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Redirected 'local' flag through IOSwitchboard.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
c8d43f918981d06a73662cf1be61081915816ca5 
  src/slave/containerizer/mesos/containerizer.cpp 
d9e6bb4e4d62e6aa542c0bc7e9a5e0c6dcc670e5 
  src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
  src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52866: Refactored HealthChecker::reschedule to take duration as an argument.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:30 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52866/
> ---
> 
> (Updated Nov. 23, 2016, 12:30 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> To facilitate code reuse, HealthChecker::reschedule() is generalized.
> This will become even more valuable when we add pause/resume functions.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/52866/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54004: Renamed functions in HealthChecker for clarity.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54004/
> ---
> 
> (Updated Nov. 23, 2016, 12:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use descriptive function names instead of underscore
> prefixes for functions in HealthChecker.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
> 
> Diff: https://reviews.apache.org/r/54004/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53611: Improved comments around health checks in mesos.proto.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 4:41 p.m.)


Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and Till 
Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
  include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53610: Added health checks documentation.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 4:41 p.m.)


Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and Till 
Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/health-checks.md PRE-CREATION 
  docs/home.md 71bcb3e17ffa3119269591647fc055bab58850b4 

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


Testing
---

https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1


Thanks,

Alexander Rukletsov

Re: Review Request 53610: Added health checks documentation.

[Alexander Rukletsov]

> On Nov. 10, 2016, 2:44 p.m., Gastón Kleiman wrote:
> > docs/health-checks.md, lines 242-243
> > 
> >
> > s/docker/the Docker/
> > s/mesos/the Mesos/

Unfortunately, we are very inconsistent, see for example 
https://mesos.apache.org/documentation/latest/containerizer/ or 
https://mesos.apache.org/documentation/latest/docker-containerizer/ . There are 
all kind of combinations there, hence I drop the issue for now.


- Alexander


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


On Nov. 24, 2016, 11:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53610/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md PRE-CREATION 
>   docs/home.md 71bcb3e17ffa3119269591647fc055bab58850b4 
> 
> Diff: https://reviews.apache.org/r/53610/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53704: Added a level of indirection for logger through an IO Switchboard.

[Kevin Klues]

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

(Updated Nov. 24, 2016, 4:29 p.m.)


Review request for mesos, Benjamin Hindman, Jie Yu, and Vinod Kone.


Changes
---

Rebased on master to accommodate addition of `user` parameter to 
`logger->parpare()`


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


Repository: mesos


Description
---

The purpose of this component is to feed stdin to a container from an
external source, as well as redirect the stdin/stdout of a container
to multiple targets.

In this commit, we simply add the IOSwitchboard as a component that
interposes on the fds set up to communicate between the logger and a
container.

In the future, we will expand this component to (optionaly) launch a
sidecar HTTP server process which will be responsible for handling
'ATTACH_CONTAINER_INPUT' and 'ATTACH_CONTAINER_OUTPUT' calls on behalf
of a container to redirect the stdin/stdout/sderr of a container to
external clients.


Diffs (updated)
-

  src/CMakeLists.txt aef9ae6d2872dc15725c01ce85b657965485605f 
  src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
  src/slave/containerizer/mesos/containerizer.hpp 
c8d43f918981d06a73662cf1be61081915816ca5 
  src/slave/containerizer/mesos/containerizer.cpp 
d9e6bb4e4d62e6aa542c0bc7e9a5e0c6dcc670e5 
  src/slave/containerizer/mesos/io/switchboard.hpp PRE-CREATION 
  src/slave/containerizer/mesos/io/switchboard.cpp PRE-CREATION 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
c369b313b5ed375cc8790c4f6e5cc22f6f9095c1 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
fa773c8badf4ddca11b5dfe8033b5ffb16dba213 

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


Testing
---

GTEST_FILTER="" make -j check
sudo src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 53610: Added health checks documentation.

[Alexander Rukletsov]

> On Nov. 24, 2016, 3:10 p.m., Till Toenshoff wrote:
> > docs/health-checks.md, line 19
> > 
> >
> > s/reimplement/re-implement/ ?

Looks like the original version is corret. See 
http://data.grammarbook.com/blog/hyphens/hyphens-with-the-prefix-re/


- Alexander


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


On Nov. 24, 2016, 11:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53610/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md PRE-CREATION 
>   docs/home.md 71bcb3e17ffa3119269591647fc055bab58850b4 
> 
> Diff: https://reviews.apache.org/r/53610/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52865: Refactored HealthChecker to never stop health checking.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On Nov. 23, 2016, 12:50 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52865/
> ---
> 
> (Updated Nov. 23, 2016, 12:50 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Mahler, Gastón Kleiman, 
> and haosdent huang.
> 
> 
> Bugs: MESOS-5963
> https://issues.apache.org/jira/browse/MESOS-5963
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prior to this patch, HealthChecker would stop performing health
> checks after it marks the task for kill. Since tasks' lifecycle
> is managed by scheduler-executor, HealthChecker should never stop
> health checking on its own.
> 
> Allowing health checks to run forever enables the scheduler
> make the decision about how to deal with unhealthy tasks.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp eefbc0c2936eb93044c2d8a1b50155171f398562 
>   src/health-check/health_checker.hpp 
> 837d1358e418d21536da488e4a23cbfa41db6060 
>   src/health-check/health_checker.cpp 
> af5500be249c74a4a5e64bf38dea607173e2f998 
>   src/launcher/default_executor.cpp f4e1ea4816fee9a2f6d59bb534933b66013f11cf 
>   src/launcher/executor.cpp ce0b199551447504bb95743df4ce9ec4a0443cd4 
> 
> Diff: https://reviews.apache.org/r/52865/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/52873/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54062: Added logic to validate FrameworkInfo roles.

[Jay Guo]

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

(Updated Nov. 24, 2016, 3:30 p.m.)


Review request for mesos and Guangya Liu.


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


Repository: mesos


Description
---

For backwards compatibility, we keep role in FrameworkInfo and do
necessary validation, which complies with following matrix:

-- MULTI_ROLE is NOT set -
|---|-|
|Roles  |No Roles |
|---|---|-|
|Role   | Error |  None   |
|---|---|-|
|No Role| Error |  None   |
|---|---|-|

--- MULTI_ROLE is set 
|---|-|
|Roles  |No Roles |
|---|---|-|
|Role   | Error |  Error  |
|---|---|-|
|No Role| None  |  None   |
|---|---|-|


Diffs
-

  include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
  src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
  src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
  src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 

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


Testing (updated)
---

make check


Thanks,

Jay Guo

Review Request 54062: Added logic to validate FrameworkInfo roles.

[Jay Guo]

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

Review request for mesos and Guangya Liu.


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


Repository: mesos


Description
---

For backwards compatibility, we keep role in FrameworkInfo and do
necessary validation, which complies with following matrix:

-- MULTI_ROLE is NOT set -
|---|-|
|Roles  |No Roles |
|---|---|-|
|Role   | Error |  None   |
|---|---|-|
|No Role| Error |  None   |
|---|---|-|

--- MULTI_ROLE is set 
|---|-|
|Roles  |No Roles |
|---|---|-|
|Role   | Error |  Error  |
|---|---|-|
|No Role| None  |  None   |
|---|---|-|


Diffs
-

  include/mesos/roles.hpp 1e6954447b821c2cf0dfa11373480161a0da5ae6 
  src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
  src/master/master.cpp e03a2e8025943825a2902102c43dc0eb66bacb6a 
  src/tests/role_tests.cpp 47fe0f06fb7728694445a62ccfe818fd9bf37c2c 

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


Testing
---


Thanks,

Jay Guo

Review Request 54061: Augmented FrameworkInfo to include repeated roles.

[Jay Guo]

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

Review request for mesos and Guangya Liu.


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


Repository: mesos


Description
---

This is the initial patch to introduce multi-role frameworks.
A new section `repeated string roles` is added to FrameworkInfo.
Old `optional string role` is kept for backwards compatability.
To distinguish single-role and multi-role frameworks, a new framework
capability `MULTI_ROLE` is introduced. See MESOS-6628 for more info.


Diffs
-

  include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
  include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 

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


Testing
---


Thanks,

Jay Guo

Re: Review Request 53611: Improved comments around health checks in mesos.proto.

[Till Toenshoff]

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


Fix it, then Ship it!





include/mesos/mesos.proto (line 408)


s/After the/After this/?

really unsure here :)



include/mesos/v1/mesos.proto (line 408)


see above?


- Till Toenshoff


On Nov. 24, 2016, 11:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53611/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
>   include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 
> 
> Diff: https://reviews.apache.org/r/53611/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53610: Added health checks documentation.

[Till Toenshoff]

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


Fix it, then Ship it!




Great work Alex, love it. As others stated as well, I am not a native speaker 
hence my intuitions may be off...


docs/health-checks.md (line 19)


s/reimplement/re-implement/ ?



docs/health-checks.md (line 40)


s/operating incorrectly/is not responsive/ ?



docs/health-checks.md (line 50)


s/agent/agent host/ or s/agent/agent node/ ?



docs/health-checks.md (line 51)


s/who/which/



docs/health-checks.md (line 79)


s/occured/occurred/



docs/health-checks.md (line 143)


s/return codes/status codes/



docs/health-checks.md (line 212)


s/For example/As an example/ ?


- Till Toenshoff


On Nov. 24, 2016, 11:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53610/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md PRE-CREATION 
>   docs/home.md 71bcb3e17ffa3119269591647fc055bab58850b4 
> 
> Diff: https://reviews.apache.org/r/53610/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 53611: Improved comments around health checks in mesos.proto.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53612, 53613, 53614, 53610, 53611]

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 Nov. 24, 2016, 11:29 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53611/
> ---
> 
> (Updated Nov. 24, 2016, 11:29 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
>   include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 
> 
> Diff: https://reviews.apache.org/r/53611/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54053: Updated 'io::redirect()' to take an optional vector of callback hooks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54053]

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 Nov. 24, 2016, 6:55 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54053/
> ---
> 
> (Updated Nov. 24, 2016, 6:55 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-6639
> https://issues.apache.org/jira/browse/MESOS-6639
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These callback hooks will be invoked before passing any data read from
> the 'from' file descriptor on to the 'to' file descriptor.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> eec5efd7e6b71a783f2bb40826054d0488cee71f 
>   3rdparty/libprocess/src/io.cpp d930ceebc90468e082b984e41385549f906dc6ae 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 66b610e35fa9f6a1738c77d181d76dca3921e6fb 
> 
> Diff: https://reviews.apache.org/r/54053/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j40 check
> GTEST_FILTER="IOTest.Redirect" 3rdparty/libprocess/libprocess-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53954: Made Zk error message more accurate.

[Alexander Rukletsov]

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


Fix it, then Ship it!




I'll fix the outstanding issue and will commit it for you.


src/zookeeper/group.cpp (line 608)


This one as well


- Alexander Rukletsov


On Nov. 23, 2016, 4:32 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53954/
> ---
> 
> (Updated Nov. 23, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Zk node creation fails, we should report the full Zk path that we
> tried to create.
> 
> 
> Diffs
> -
> 
>   src/zookeeper/group.cpp 1df4b54883d451fe36f132af45f2f7c77f3f221b 
> 
> Diff: https://reviews.apache.org/r/53954/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 53832: Fixing broken link for the app frameowrks development guide page.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Nov. 17, 2016, 12:32 a.m., Miguel Bernadin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53832/
> ---
> 
> (Updated Nov. 17, 2016, 12:32 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixing broken link for the app frameowrks development guide. 
> http://mesos.apache.org/documentation/latest/app-framework-development-guide/
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> de92c7570cd0d0ac8639ce50a79e5158844ac53c 
> 
> Diff: https://reviews.apache.org/r/53832/diff/
> 
> 
> Testing
> ---
> 
> Validated the files exist locally and contained the correct name. Ensured the 
> correction followed the existing standard.
> 
> 
> Thanks,
> 
> Miguel Bernadin
> 
>

Re: Review Request 53612: Fixed minor style issues in docs.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:30 a.m.)


Review request for mesos, Joerg Schad and Neil Conway.


Repository: mesos


Description (updated)
---

According to our markdown style guide, notes should be decorated as
**NOTE:** and not __NOTE:__ and formatted without extra indentation
or extra symbols.


Diffs
-

  docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 
  docs/reservation.md 20db7c97d5c9d651c6d79864e5093c2fd4bdfc04 
  docs/sandbox.md af0036df05b31bc0bc028d283b7c5a0c60674327 
  docs/ssl.md 2f7d928ef94b9fb611e071b01755c002554bad40 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53611: Improved comments around health checks in mesos.proto.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:29 a.m.)


Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and Till 
Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 380575904a5a29ee53bdc87ae8791ed14e3cafca 
  include/mesos/v1/mesos.proto 5b542ff9cd0b5f301c088d3f4b2fe17ae760d368 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53614: Updated the markdown style guide with headings capitalization.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:28 a.m.)


Review request for mesos, Joerg Schad and Neil Conway.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/markdown-style-guide.md 667d6dfbcf6c0864085cdfb19f4e53fb49a49c44 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53610: Added health checks documentation.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:29 a.m.)


Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and Till 
Toenshoff.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/health-checks.md PRE-CREATION 
  docs/home.md 71bcb3e17ffa3119269591647fc055bab58850b4 

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


Testing
---

https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1


Thanks,

Alexander Rukletsov

Re: Review Request 53613: Fixed style issues in quota.md.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:28 a.m.)


Review request for mesos, Joerg Schad and Neil Conway.


Repository: mesos


Description
---

Correct caption hierarchy, update headings to conform to
title case, and ensure two blank lines between sections.


Diffs (updated)
-

  docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53612: Fixed minor style issues in docs.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:27 a.m.)


Review request for mesos, Joerg Schad and Neil Conway.


Repository: mesos


Description
---

According to our markdown style guide, Notes should be decorated as
**NOTE:** and not __NOTE:__ and formatted without extra indentation
or extra symbols.


Diffs (updated)
-

  docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 
  docs/reservation.md 20db7c97d5c9d651c6d79864e5093c2fd4bdfc04 
  docs/sandbox.md af0036df05b31bc0bc028d283b7c5a0c60674327 
  docs/ssl.md 2f7d928ef94b9fb611e071b01755c002554bad40 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53612: Fixed minor style issues in docs.

[Alexander Rukletsov]

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

(Updated Nov. 24, 2016, 11:27 a.m.)


Review request for mesos, Joerg Schad and Neil Conway.


Repository: mesos


Description
---

According to our markdown style guide, Notes should be decorated as
**NOTE:** and not __NOTE:__ and formatted without extra indentation
or extra symbols.


Diffs
-

  docs/quota.md d7e8d58eeeac9d9ffc1a6f7614c7ddce6d7a9b89 
  docs/reservation.md 20db7c97d5c9d651c6d79864e5093c2fd4bdfc04 
  docs/sandbox.md af0036df05b31bc0bc028d283b7c5a0c60674327 
  docs/ssl.md 2f7d928ef94b9fb611e071b01755c002554bad40 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 53610: Added health checks documentation.

[Alexander Rukletsov]

> On Nov. 22, 2016, 8:34 p.m., Neil Conway wrote:
> > docs/health-checks.md, line 23
> > 
> >
> > I think this would benefit from some more discussion of the high-level 
> > architecture of Mesos-native health checks. For example:
> > 
> > * the traditional "scheduler health check" pattern involves a single 
> > scheduler node and a collection of agents; Mesos-native health checks run 
> > on the agent. This improves scalability but means that detecting network 
> > faults is a separate concern.
> > * when a task fails Mesos-native health checks, what happens to it? how 
> > does the framework scheduler learn about this?
> > * what happens if a task is running on a partitioned agent -- will it 
> > still be health-checked? If those health-checks fail, will the task be 
> > terminated?
> > 
> > Some of this is discussed below, but I think it would be better to 
> > briefly discuss it at the beginning of the document to set context for what 
> > follows.

I've rearranged the text a bit: brought delivery and failure sections before 
the anatomy of a health checked, merged them together, and [added 
drama](https://www.youtube.com/watch?v=316AzLYfAzw) about network partitions.


- Alexander


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


On Nov. 20, 2016, 6:52 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53610/
> ---
> 
> (Updated Nov. 20, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, haosdent huang, Neil Conway, and 
> Till Toenshoff.
> 
> 
> Bugs: MESOS-5597
> https://issues.apache.org/jira/browse/MESOS-5597
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/health-checks.md PRE-CREATION 
>   docs/home.md a5811480de050352dca6c0f7e4e64d3d2351c2d5 
> 
> Diff: https://reviews.apache.org/r/53610/diff/
> 
> 
> Testing
> ---
> 
> https://gist.github.com/rukletsov/7200c36b2fd1e81f78f2583e68b31fd1
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 54038: Added new hook for covering executor and task environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54038]

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 Nov. 23, 2016, 7:26 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54038/
> ---
> 
> (Updated Nov. 23, 2016, 7:26 p.m.)
> 
> 
> Review request for mesos, Adam B, Gastón Kleiman, Kapil Arya, and Joseph Wu.
> 
> 
> Bugs: MESOS-6396
> https://issues.apache.org/jira/browse/MESOS-6396
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For being able to supply environment additions for both, a task and
> its executor separately we need to introduce a new hook as the
> existing ones slavePreLaunchDockerHook (deprecated) as well as
> slavePreLaunchDockerEnvironmentDecorator do not allow for this.
> 
> This new hook will likely allow for further additions like e.g.
> adding volumes without having to adapt the signature but only the
> returned proto message TaskExecutorDecoratorInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp f0606e3a68fa179cf7ea036f10563ef47c2aefa7 
>   include/mesos/module/hook.proto PRE-CREATION 
>   src/Makefile.am 5e0b8406f7f624bd8b03ff76b887f20e22fc66e0 
>   src/hook/manager.hpp 5ecfcab48da808c84d36f9bcfcb5a8e0ad2167e5 
>   src/hook/manager.cpp 24885226a788a7abd851e12b527f74fa972ec935 
>   src/slave/containerizer/docker.cpp ccabf99f305d7874e1c46bc618ea74341eb281ef 
> 
> Diff: https://reviews.apache.org/r/54038/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> *WIP - functional test pending - unit tests pending - WIP*
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>