Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Jie Yu 

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


Ship it!




Ship It!

- Jie Yu


On July 2, 2016, 9:50 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated July 2, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Jie Yu 


> On July 2, 2016, 5:46 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 580
> > 
> >
> > Please use UUID::random here.
> 
> Haris Choudhary wrote:
> I've made the changes. But consider if both UUID's were the same as a 
> result of random, the test would fail.
> However I think its super rare for both to be generated as the same.

UUID stands for universal unique id. Many components in Mesos will break if the 
uniqueness assumption is broken. I won't be too worried.


- Jie


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


On July 2, 2016, 9:50 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated July 2, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Mesos ReviewBot 

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



Patch looks great!

Reviews applied: [49414]

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 July 2, 2016, 9:50 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated July 2, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Haris Choudhary 


> On July 2, 2016, 5:46 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 580
> > 
> >
> > Please use UUID::random here.

I've made the changes. But consider if both UUID's were the same as a result of 
random, the test would fail.
However I think its super rare for both to be generated as the same.


> On July 2, 2016, 5:46 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, line 582
> > 
> >
> > Ditto on using UUID::random

See previous comment for UUID


> On July 2, 2016, 5:46 a.m., Jie Yu wrote:
> > src/tests/containerizer/mesos_containerizer_tests.cpp, lines 591-593
> > 
> >
> > NO need for those default variables.

I tried removing None() here but the code wouldnt compile because the arguments 
are required. Could you please elaborate what is required instead? Thanks


- Haris


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


On July 2, 2016, 9:50 p.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated July 2, 2016, 9:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Haris Choudhary 

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

(Updated July 2, 2016, 9:50 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description (updated)
---

Added Executor PID in /containers endpoint. Also Added Test Cases.


Diffs (updated)
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/slave/containerizer/mesos/containerizer.cpp 
a96b382f22886362a1159e1166dfe041072985ba 
  src/slave/containerizer/mesos/launcher.hpp 
05320f462653c31fc2f093d6c67e2182e9c794fa 
  src/slave/containerizer/mesos/launcher.cpp 
ff675262af8947b89f8099828665e5e5d86491d8 
  src/slave/containerizer/mesos/linux_launcher.hpp 
89bb2958a41dffe4ade9c2492b9a7412f90a432d 
  src/slave/containerizer/mesos/linux_launcher.cpp 
5028854fa003615f158120e030866b7ec4402b66 
  src/tests/containerizer/launcher.hpp c352634c4766d289706c7cc738677619d7d02ccd 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
6c14f6e20b4d14d5ed095670673571739101b0e4 

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


Testing
---

make and make check.


Thanks,

Haris Choudhary

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-02 Thread Guangya Liu 

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



Seems most comments are style issue, so I would suggest that you go through the 
document here 
https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md to get 
more details.


include/mesos/mesos.proto (line 1873)


Period to the end



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


ditto



src/slave/containerizer/mesos/launcher.hpp (line 79)


period to the end



src/slave/containerizer/mesos/launcher.cpp (lines 175 - 176)


new line here



src/slave/containerizer/mesos/linux_launcher.cpp (lines 372 - 373)


new line here



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 567 - 568)


period to the end.

Ditto for the following of all the new added comments.


- Guangya Liu


On 六月 30, 2016, 12:51 a.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated 六月 30, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> In order to greatly simplify the implementation for the Mesos CLI's container 
> plugin, we need the executor PID (Process ID) to be exposed in the 
> /containers endpoint. [Mesos CLI 
> Epic](https://issues.apache.org/jira/browse/MESOS-5676)
> This change will introduce the pid for an executor if it was launched by the 
> mesos containerizer in the /containers endpoint of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-01 Thread Mesos ReviewBot 

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



Bad patch!

Reviews applied: [49414]

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

Error:
2016-07-02 06:28:20 URL:https://reviews.apache.org/r/49414/diff/raw/ 
[7348/7348] -> "49414.patch" [1]
Total errors found: 0
Checking 7 files
Error: No line in the commit message summary may exceed 72 characters.

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

- Mesos ReviewBot


On June 30, 2016, 12:51 a.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated June 30, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> In order to greatly simplify the implementation for the Mesos CLI's container 
> plugin, we need the executor PID (Process ID) to be exposed in the 
> /containers endpoint. [Mesos CLI 
> Epic](https://issues.apache.org/jira/browse/MESOS-5676)
> This change will introduce the pid for an executor if it was launched by the 
> mesos containerizer in the /containers endpoint of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>

Re: Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

2016-07-01 Thread Jie Yu 

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




src/slave/containerizer/mesos/launcher.hpp (line 78)


Add a new line above.



src/slave/containerizer/mesos/launcher.hpp (line 81)


indentation for parameters should be 4



src/slave/containerizer/mesos/launcher.hpp (line 115)


Ditto.



src/slave/containerizer/mesos/linux_launcher.hpp (line 55)


Ditto.



src/slave/containerizer/mesos/linux_launcher.cpp (line 367)


2 lines apart between top level functions.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 458)


Please move this right above `TEST_F(MesosLauncherStatusTest ...`

And please move all the stuff you just added to the end of this file.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 566)


2 lines apart please.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 580)


Please use UUID::random here.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 582)


Ditto on using UUID::random



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 585 - 593)


INdentation here should be 4.



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 591 - 593)


NO need for those default variables.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 599)


Please use AWAIT_READY(validStatus) here



src/tests/containerizer/mesos_containerizer_tests.cpp (line 601)


you can use `validStatus->executor_id`



src/tests/containerizer/mesos_containerizer_tests.cpp (line 603)


No need for back slash in the end



src/tests/containerizer/mesos_containerizer_tests.cpp (line 604)


Indentation here should be 2. See style guide



src/tests/containerizer/mesos_containerizer_tests.cpp (line 610)


2 lines apart, please.


- Jie Yu


On June 30, 2016, 12:51 a.m., Haris Choudhary wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49414/
> ---
> 
> (Updated June 30, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5737
> https://issues.apache.org/jira/browse/MESOS-5737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Executor PID in /containers endpoint. Also Added Test Cases.
> 
> In order to greatly simplify the implementation for the Mesos CLI's container 
> plugin, we need the executor PID (Process ID) to be exposed in the 
> /containers endpoint. [Mesos CLI 
> Epic](https://issues.apache.org/jira/browse/MESOS-5676)
> This change will introduce the pid for an executor if it was launched by the 
> mesos containerizer in the /containers endpoint of an agent.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
>   include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a96b382f22886362a1159e1166dfe041072985ba 
>   src/slave/containerizer/mesos/launcher.hpp 
> 05320f462653c31fc2f093d6c67e2182e9c794fa 
>   src/slave/containerizer/mesos/launcher.cpp 
> ff675262af8947b89f8099828665e5e5d86491d8 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 89bb2958a41dffe4ade9c2492b9a7412f90a432d 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launcher.hpp 
> c352634c4766d289706c7cc738677619d7d02ccd 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 6c14f6e20b4d14d5ed095670673571739101b0e4 
> 
> Diff: https://reviews.apache.org/r/49414/diff/
> 
> 
> Testing
> ---
> 
> make and make check.
> 
> 
> Thanks,
> 
> Haris Choudhary
> 
>