Re: Review Request 46799: Added capabilities support to mesos-execute.

[Joris Van Remoortere]

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



Closing this review as Benjamin has followed up with separate reviews. Please 
see our 
[guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md)
 for reopening reviews.

- Joris Van Remoortere


On May 13, 2016, 4:46 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 13, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [46799, 46798, 46371, 46370, 46369]

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

Error:
2016-05-13 18:08:59 URL:https://reviews.apache.org/r/46371/diff/raw/ 
[18333/18333] -> "46371.patch" [1]
error: patch failed: src/Makefile.am:2045
error: src/Makefile.am: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/13051/console

- Mesos ReviewBot


On May 13, 2016, 4:46 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 13, 2016, 4:46 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

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

(Updated May 13, 2016, 4:46 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

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

(Updated May 13, 2016, 12:50 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [46799, 46798, 46371, 46370, 46369]

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

Error:
2016-05-06 21:48:11 URL:https://reviews.apache.org/r/46371/diff/raw/ 
[18318/18318] -> "46371.patch" [1]
error: patch failed: src/Makefile.am:2045
error: src/Makefile.am: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/12921/console

- Mesos ReviewBot


On May 6, 2016, 5:06 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 6, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

> On April 29, 2016, 12:19 a.m., Qian Zhang wrote:
> > src/cli/execute.cpp, lines 172-175
> > 
> >
> > Can we only add this flag in Linux? Then user will not see this flag at 
> > all when running `mesos-execute` in other platorms.
> 
> Jojy Varghese wrote:
> I avoided that logic as it would mean spreading `ifdef` all over the file.
> 
> Qian Zhang wrote:
> Right, but I think that is the common way to handle the platform-specific 
> logic, you can take a look at: 
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp,
>  in this file, we use `#ifdef __linux__` for all the isolators which can be 
> only enabled on Linux.

I have looked at the platform speific ifdefs in the code and I am not sure if 
that is the right way to do platform specific code. Linux kernel is a good 
example of how it actually should be done. But I am changing the code in cli 
for now for uniformity.


- Jojy


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


On May 6, 2016, 5:06 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated May 6, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

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

(Updated May 6, 2016, 5:06 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Qian Zhang]

> On April 29, 2016, 8:19 a.m., Qian Zhang wrote:
> > src/cli/execute.cpp, lines 172-175
> > 
> >
> > Can we only add this flag in Linux? Then user will not see this flag at 
> > all when running `mesos-execute` in other platorms.
> 
> Jojy Varghese wrote:
> I avoided that logic as it would mean spreading `ifdef` all over the file.

Right, but I think that is the common way to handle the platform-specific 
logic, you can take a look at: 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/containerizer.cpp,
 in this file, we use `#ifdef __linux__` for all the isolators which can be 
only enabled on Linux.


- Qian


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


On April 30, 2016, 2:41 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated April 30, 2016, 2:41 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46369, 46370, 46371, 46798, 46799]

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

- Mesos ReviewBot


On April 29, 2016, 6:41 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated April 29, 2016, 6:41 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

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

(Updated April 29, 2016, 6:41 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs (updated)
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

> On April 29, 2016, 12:19 a.m., Qian Zhang wrote:
> > src/cli/execute.cpp, lines 172-175
> > 
> >
> > Can we only add this flag in Linux? Then user will not see this flag at 
> > all when running `mesos-execute` in other platorms.

I avoided that logic as it would mean spreading `ifdef` all over the file.


- Jojy


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


On April 28, 2016, 9:49 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated April 28, 2016, 9:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 46799: Added capabilities support to mesos-execute.

[Qian Zhang]

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




src/cli/execute.cpp (lines 43 - 46)


I think we should add this before `#include ` to keep 
them in alphabet order.



src/cli/execute.cpp (lines 172 - 175)


Can we only add this flag in Linux? Then user will not see this flag at all 
when running `mesos-execute` in other platorms.


- Qian Zhang


On April 29, 2016, 5:49 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46799/
> ---
> 
> (Updated April 29, 2016, 5:49 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces two flags: `user` and `capabilities`.
> 
>user:  used to specify the user name for the command.
>capabilities: comma separated list of capabilities the task
>  requires.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
> 
> Diff: https://reviews.apache.org/r/46799/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 46799: Added capabilities support to mesos-execute.

[Jojy Varghese]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This change introduces two flags: `user` and `capabilities`.

   user:  used to specify the user name for the command.
   capabilities: comma separated list of capabilities the task
 requires.


Diffs
-

  src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 

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


Testing
---

make check.


Thanks,

Jojy Varghese