date:20150903

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread Timothy Chen


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

Ship it!


Ship It!


src/docker/docker.cpp (line 203)


We remove the overflow components


- Timothy Chen


On Sept. 4, 2015, 2:27 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 4, 2015, 2:27 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37669]

All tests passed.

- Mesos ReviewBot


On Sept. 4, 2015, 2:27 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 4, 2015, 2:27 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37873: Add quiesce logic in allocator

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37532, 37873]

All tests passed.

- Mesos ReviewBot


On Sept. 4, 2015, 2:19 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37873/
> ---
> 
> (Updated Sept. 4, 2015, 2:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add quiesce logic in allocator
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> cb4020dea897ef198cd9898cabecf61edfade834 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37873/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

2015-09-03 Thread haosdent huang



> On Aug. 31, 2015, 6:46 p.m., Joseph Wu wrote:
> > For any future CMake reviews, could you also add Artem (hartem) and I 
> > (kaysoky) as reviewers?
> > Thanks in advance :)

Sure? thank you very much.


- haosdent


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


On Aug. 30, 2015, 1:42 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> ---
> 
> (Updated Aug. 30, 2015, 1:42 p.m.)
> 
> 
> Review request for mesos and Alex Clemmer.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake macro VsBuildCommand in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37273/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

2015-09-03 Thread Guangya Liu


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

(Updated Sept. 4, 2015, 3:35 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

removed circular dep -- @vinodkone


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


Repository: mesos


Description
---

This is just part of MESOS-3037, this patch only add the interface
of QUIESCE call.


Diffs
-

  include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
  include/mesos/scheduler/scheduler.proto 
89daf8a6b74057ee156b3ad691397e76fcb835b8 
  src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 

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


Testing
---


Thanks,

Guangya Liu

Review Request 38118: Fix webui display problems when starting lots of tasks with small cpu value.

2015-09-03 Thread haosdent huang


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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Fix webui display problems when starting lots of tasks with small cpu value.


Diffs
-

  src/webui/master/static/home.html ce8ca192235c224715c01fef0b8ddb187dc0a827 
  src/webui/master/static/js/controllers.js 
3445028138fd621d0e3a5427668c5eca7db5224f 

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


Testing
---

manually test


Thanks,

haosdent huang

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread haosdent huang



> On Sept. 3, 2015, 3:55 p.m., Jan Schlicht wrote:
> > src/tests/containerizer/docker_tests.cpp, line 229
> > 
> >
> > This is a test of the special case with a "1.7.1.fc22" version string. 
> > How about an additional test of the general case like "1.7.1"?

Sure, already updated. Thank you very much.


- haosdent


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


On Sept. 4, 2015, 2:27 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 4, 2015, 2:27 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread haosdent huang


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

(Updated Sept. 4, 2015, 2:27 a.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.


Changes
---

update according @Jan's reviews


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


Repository: mesos


Description
---

Ignore overflow docker components in version parsing.


Diffs (updated)
-

  src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
  src/tests/containerizer/docker_tests.cpp 
cfad36850b40ade7a41f5b92255320ca1ed1bf93 

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


Testing
---

# Add new test case
sudo ./bin/mesos-tests.sh 
--gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose


Thanks,

haosdent huang

Re: Review Request 37532: Add QUIESCE call interface to the scheduler

2015-09-03 Thread Guangya Liu


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

(Updated 九月 4, 2015, 2:20 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

This is just part of MESOS-3037, this patch only add the interface
of QUIESCE call.


Diffs (updated)
-

  include/mesos/scheduler.hpp ee198b6955882f4f31466ca05429ca16fbf2f5cd 
  include/mesos/scheduler/scheduler.proto 
89daf8a6b74057ee156b3ad691397e76fcb835b8 
  src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/sched/sched.cpp 012af0508eeceeccd168b29f36fa258d20b28c21 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37873: Add quiesce logic in allocator

2015-09-03 Thread Guangya Liu


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

(Updated 九月 4, 2015, 2:19 a.m.)


Review request for mesos, Alexander Rukletsov, Ben Mahler, Jie Yu, and Vinod 
Kone.


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


Repository: mesos


Description
---

Add quiesce logic in allocator


Diffs (updated)
-

  include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
  src/master/allocator/mesos/allocator.hpp 
aa55755a9c3250579e9366bdbc17a2449e95d659 
  src/master/allocator/mesos/hierarchical.hpp 
cb4020dea897ef198cd9898cabecf61edfade834 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 37873: Add quiesce logic in allocator

2015-09-03 Thread Guangya Liu



> On 八月 30, 2015, 5:10 p.m., Robert Lacroix wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 153
> > 
> >
> > Are we quiescing resources or rather offers? I think it should be 
> > called `quiesceOffers` so it's symmetrical to `reviveOffers`.
> 
> Alexander Rukletsov wrote:
> Technically, allocator does not know anything about offers, only about 
> allocations. This may change soon (we think about pulling offers management 
> into allocator), but until then I find this naming misleading.

Alex, what about addressing this in future patches after pulling offers 
management into allocator landed? Thanks.


- Guangya


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


On 九月 4, 2015, 2:19 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37873/
> ---
> 
> (Updated 九月 4, 2015, 2:19 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Ben Mahler, Jie Yu, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add quiesce logic in allocator
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> cb4020dea897ef198cd9898cabecf61edfade834 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/mesos.hpp 906948d459b5a88a4ad7952801eb8c540b58c569 
> 
> Diff: https://reviews.apache.org/r/37873/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38117: Export per container SNMP statistics

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38117]

All tests passed.

- Mesos ReviewBot


On Sept. 4, 2015, 1:17 a.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38117/
> ---
> 
> (Updated Sept. 4, 2015, 1:17 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.
> 
> 
> Bugs: MESOS-3365
> https://issues.apache.org/jira/browse/MESOS-3365
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These stats are those we get by `netstat -s`, they are important for diagnose 
> networking issues.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
>   src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
>   src/slave/flags.cpp 49fb0007356eedf7095d34a1312e99bafa8fdc4d 
> 
> Diff: https://reviews.apache.org/r/38117/diff/
> 
> 
> Testing
> ---
> 
> Manual tests
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Vinod Kone


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

Ship it!


Ship It!

- Vinod Kone


On Sept. 4, 2015, 1:18 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38050/
> ---
> 
> (Updated Sept. 4, 2015, 1:18 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1831
> https://issues.apache.org/jira/browse/MESOS-1831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master should send PingSlaveMessage instead of "PING"; Slave should accept 
> PingSlaveMessage but not "PING" message;
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc 
>   src/tests/partition_tests.cpp b7030ad 
>   src/tests/slave_recovery_tests.cpp 4d137e0 
>   src/tests/slave_tests.cpp 2411918 
> 
> Diff: https://reviews.apache.org/r/38050/diff/
> 
> 
> Testing
> ---
> 
> Test2: Start master and slave to debug the PingSlaveMessage. Passed!
> Test1: Run make check successfully!
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Yong Qiao Wang


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

(Updated 九月 4, 2015, 1:18 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Master should send PingSlaveMessage instead of "PING"; Slave should accept 
PingSlaveMessage but not "PING" message;


Diffs (updated)
-

  src/master/master.cpp 56bcbcc 
  src/tests/partition_tests.cpp b7030ad 
  src/tests/slave_recovery_tests.cpp 4d137e0 
  src/tests/slave_tests.cpp 2411918 

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


Testing
---

Test2: Start master and slave to debug the PingSlaveMessage. Passed!
Test1: Run make check successfully!


Thanks,

Yong Qiao Wang

Review Request 38117: Export per container SNMP statistics

2015-09-03 Thread Cong Wang


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

Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.


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


Repository: mesos


Description
---

These stats are those we get by `netstat -s`, they are important for diagnose 
networking issues.


Diffs
-

  include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
  src/slave/containerizer/isolators/network/port_mapping.hpp 
4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
  src/slave/containerizer/isolators/network/port_mapping.cpp 
34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
  src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
  src/slave/flags.cpp 49fb0007356eedf7095d34a1312e99bafa8fdc4d 

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


Testing
---

Manual tests


Thanks,

Cong Wang

Re: Review Request 37168: MESOS-3063

2015-09-03 Thread Michael Park



> On Sept. 1, 2015, 3:34 a.m., Michael Park wrote:
> > Just a bunch of high-level questions here.
> > 
> > It looks like you followed the pattern for 
> > `src/examples/test_framework.cpp`.
> > I think it would be probably better to follow 
> > `src/examples/persistent_volume_framework.cpp` since
> > 
> > 1. It's likely that people will visit dynamic reservation and persistent 
> > volumes together.
> > 2. There are many components (e.g. authorization, implicit status update 
> > acknowledgements, etc) included in the test_framework that are there for 
> > demo purposes. I think it would be better to keep this example's scope to 
> > dynamic reservation, and it should lead us to a simpler example.
> > 
> > Also, do we need the custom executor for some reason? It doesn't seem to be 
> > doing much currently?
> > 
> > Another high-level question is, how would you describe what `State` 
> > represents?
> > It seems like it's associated with every offer, would it makes more sense 
> > to associate it with a task instead?
> 
> Klaus Ma wrote:
> 1. Yes, I followed test_framework.cpp; and I'll polish it by following 
> persistent_volume_framework.cpp.
> 2. Regarding the executor, we can use test_executor.cpp to replase.
> 3. For State, it describe whether a slave has reserved resources; it's an 
> example for the daemon on each slave host, such as NameNode.
> 
> I'll update the code to address 1 & 2; if any more comments, i'll 
> continue to address them :).

1. Great, thanks!
2. I don't think you even need `test_executor.cpp`, do you? 
`test_framework.cpp` for example doesn't use `text_executor.cpp`, just uses 
command executor I believe.
3. Makes sense to me, thanks for the explanation :)


- Michael


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


On Aug. 28, 2015, 3:31 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated Aug. 28, 2015, 3:31 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3063 (Add an example framework using dynamic reservation)
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff 
>   src/examples/dynamic_reservation_executor.cpp PRE-CREATION 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 3f56b30 
>   src/tests/flags.hpp 3644956 
>   src/tests/script.cpp bcc1fab 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Yong Qiao Wang


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

(Updated 九月 4, 2015, 1:04 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Master should send PingSlaveMessage instead of "PING"; Slave should accept 
PingSlaveMessage but not "PING" message;


Diffs (updated)
-

  3rdparty/libprocess/include/process/collect.hpp 
d07b6860a5b5cbed910b2e9e2ab2429bd70a3999 
  3rdparty/libprocess/src/tests/process_tests.cpp 
435663b10c1bfce07e8e84719aa14b5867c651c6 
  src/Makefile.am 4b643a32eb68dbd52714e9d66451dc2b62576f6d 
  src/linux/perf.hpp d10968ca670eb516bae08385fd0ddde8e8ad83b5 
  src/linux/perf.cpp 0011482cf9d920485728798518d32af0e9627724 
  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/slave/containerizer/isolators/cgroups/perf_event.hpp 
1f722ef3ef7ab7fce5542d4affae961d6cec2406 
  src/slave/containerizer/isolators/cgroups/perf_event.cpp 
7dc8b7a99074b74ade019ef4df296780650a2e4e 
  src/slave/containerizer/provisioners/backend.cpp 
6560ece14d8618878a35d1bfe27db3958da64358 
  src/slave/containerizer/provisioners/backends/copy.hpp 
2abca37ed2479d42c634c23cac8e40d515249988 
  src/slave/containerizer/provisioners/backends/copy.cpp 
b56946562525e79bef3a7387cd71f39fd0690683 
  src/tests/containerizer/perf_tests.cpp 
bef475e4b573f02b69f2c4dd6d1c6a6e10f34c38 
  src/tests/containerizer/provisioner_backend_tests.cpp 
f2498b109c910fbf753a53b4b36a88b8d779aa69 
  src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
  src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
  src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 

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


Testing
---

Test2: Start master and slave to debug the PingSlaveMessage. Passed!
Test1: Run make check successfully!


Thanks,

Yong Qiao Wang

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Yong Qiao Wang



> On 九月 3, 2015, 3:45 p.m., Vinod Kone wrote:
> > src/tests/partition_tests.cpp, line 79
> > 
> >
> > why the change to process::Message here and everywhere else?

Actually, namespace "using process::Message;" has been used in the header of 
this file, so it does not need to change to process::Message. I will update to 
roolback these changes.


> On 九月 3, 2015, 3:45 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 187-195
> > 
> >
> > any reason to not just call pong() here?

Update the pongOld function to just call pong().


- Yong Qiao


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


On 九月 3, 2015, 7:46 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38050/
> ---
> 
> (Updated 九月 3, 2015, 7:46 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1831
> https://issues.apache.org/jira/browse/MESOS-1831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master should send PingSlaveMessage instead of "PING"; Slave should accept 
> PingSlaveMessage but not "PING" message;
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
>   src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 
> 
> Diff: https://reviews.apache.org/r/38050/diff/
> 
> 
> Testing
> ---
> 
> Test2: Start master and slave to debug the PingSlaveMessage. Passed!
> Test1: Run make check successfully!
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 37929: Changed provisioner Store API and implementation so it works as a read-through cache.

2015-09-03 Thread Jiang Yan Xu



> On Aug. 31, 2015, 2:42 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc/store.hpp, lines 76-81
> > 
> >
> > Maybe not yet the guarantee on ordering since we don't support 
> > dependency in images.
> > 
> > Mainly I think if we don't have to worry about ordering, it is simpler.
> > 
> > If backends can't get away from that, we should then be more explicit 
> > on what kind of ordering we provie. BFS, first order traversal or sth else?
> 
> Jiang Yan Xu wrote:
> This comment is about what the method SHOULD do. And the TODO below says: 
> OK we are actually not doing it just yet. :)
> 
> It's important to be clear about what we WILL do in this case because 
> this is the contract between the provisioner and the store. We need to 
> determine right now where the logic should go in order to make sure the 
> interface is future proof.
> 
> The backend is told about the ordering because it itself doesn't have 
> enough information to otherwise detect it.
> 
> The ordering is basically "what overwites what in the fact of conflict" 
> so it's not BFS or how we RESOLVE the dependencies.
> 
> Chi Zhang wrote:
> I guess I wasn't clear if C, B, D, A and C, D, B, A are both acceptable 
> in your example, afte reading the comment?
> 
> Jiang Yan Xu wrote:
> Thanks. You are right I could have been more clear about this. In fact B 
> needs to go before D so I added the following comment: 
> (B before D in this example is decided by the order in which A specifies 
> its dependencies)
> 
> Chi Zhang wrote:
> is it true that the order at which A speicifies dependecies matters?

If image A depends on [B, D], it may intentionally want some files in D to 
overwrite the ones in B, right?
I am not saying it's a good practice but it's the intention of the spec to 
allow it.


- Jiang Yan


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


On Sept. 2, 2015, 5:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37929/
> ---
> 
> (Updated Sept. 2, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - i.e., It fetches images transparently when it's not in the local cache.
> - This way, the store doesn't have the sense of "localness" anymore but 
> rather it's an abstraction that provides access to all discoverable images, 
> no matter where they can be found.
> 
> Some context for motivation of this change can be found at: 
> https://reviews.apache.org/r/37881/
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/appc/store.hpp 
> e48d91be06410bfc028a7b2ed88218e13adbffee 
>   src/slave/containerizer/provisioners/appc/store.cpp 
> fbd1c535d398a4d37c30ba23f5408095c7d35b65 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37929/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37929: Changed provisioner Store API and implementation so it works as a read-through cache.

2015-09-03 Thread Jiang Yan Xu



> On Sept. 2, 2015, 5:53 p.m., Timothy Chen wrote:
> > src/slave/containerizer/provisioners/appc/store.cpp, line 235
> > 
> >
> > The wording sounds wierd, how about:
> > 
> > "Cannot match any image in store for '" + image.name()

Changed to:

```
return Failure("No image named '" + image.name() +
   "' can match the requirements");
```

Note that it's not about local availability anymore since the store is also 
responsible for retrieving it.


- Jiang Yan


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


On Sept. 2, 2015, 5:07 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37929/
> ---
> 
> (Updated Sept. 2, 2015, 5:07 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - i.e., It fetches images transparently when it's not in the local cache.
> - This way, the store doesn't have the sense of "localness" anymore but 
> rather it's an abstraction that provides access to all discoverable images, 
> no matter where they can be found.
> 
> Some context for motivation of this change can be found at: 
> https://reviews.apache.org/r/37881/
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/appc/store.hpp 
> e48d91be06410bfc028a7b2ed88218e13adbffee 
>   src/slave/containerizer/provisioners/appc/store.cpp 
> fbd1c535d398a4d37c30ba23f5408095c7d35b65 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37929/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37929: Changed provisioner Store API and implementation so it works as a read-through cache.

2015-09-03 Thread Chi Zhang



> On Aug. 31, 2015, 9:42 p.m., Chi Zhang wrote:
> > src/slave/containerizer/provisioners/appc/store.hpp, lines 76-81
> > 
> >
> > Maybe not yet the guarantee on ordering since we don't support 
> > dependency in images.
> > 
> > Mainly I think if we don't have to worry about ordering, it is simpler.
> > 
> > If backends can't get away from that, we should then be more explicit 
> > on what kind of ordering we provie. BFS, first order traversal or sth else?
> 
> Jiang Yan Xu wrote:
> This comment is about what the method SHOULD do. And the TODO below says: 
> OK we are actually not doing it just yet. :)
> 
> It's important to be clear about what we WILL do in this case because 
> this is the contract between the provisioner and the store. We need to 
> determine right now where the logic should go in order to make sure the 
> interface is future proof.
> 
> The backend is told about the ordering because it itself doesn't have 
> enough information to otherwise detect it.
> 
> The ordering is basically "what overwites what in the fact of conflict" 
> so it's not BFS or how we RESOLVE the dependencies.
> 
> Chi Zhang wrote:
> I guess I wasn't clear if C, B, D, A and C, D, B, A are both acceptable 
> in your example, afte reading the comment?
> 
> Jiang Yan Xu wrote:
> Thanks. You are right I could have been more clear about this. In fact B 
> needs to go before D so I added the following comment: 
> (B before D in this example is decided by the order in which A specifies 
> its dependencies)

is it true that the order at which A speicifies dependecies matters?


- Chi


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


On Sept. 3, 2015, 12:07 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37929/
> ---
> 
> (Updated Sept. 3, 2015, 12:07 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - i.e., It fetches images transparently when it's not in the local cache.
> - This way, the store doesn't have the sense of "localness" anymore but 
> rather it's an abstraction that provides access to all discoverable images, 
> no matter where they can be found.
> 
> Some context for motivation of this change can be found at: 
> https://reviews.apache.org/r/37881/
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/appc/store.hpp 
> e48d91be06410bfc028a7b2ed88218e13adbffee 
>   src/slave/containerizer/provisioners/appc/store.cpp 
> fbd1c535d398a4d37c30ba23f5408095c7d35b65 
>   src/tests/containerizer/appc_provisioner_tests.cpp 
> 47b66b9c30cefe8f9a8e2c1c1341776c2d235020 
> 
> Diff: https://reviews.apache.org/r/37929/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37880: Changed the Appc provisioner directory to identify a rootfs by a rootfs_id (UUID).

2015-09-03 Thread Chi Zhang


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

Ship it!


Ship It!

- Chi Zhang


On Aug. 31, 2015, 5:44 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37880/
> ---
> 
> (Updated Aug. 31, 2015, 5:44 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2796
> https://issues.apache.org/jira/browse/MESOS-2796
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - So the user can provision multiple identical rootfses in the volumes of one 
> container, a case we don't need to disallow.
> - Previously a rootfs is identified by the 'image_id' of its topmost file 
> system layer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> 41e3bf79da0854406c488855f953111e67353829 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> 3113c84b9526dd9e9e89fb9aa4ec75ed66a996c7 
> 
> Diff: https://reviews.apache.org/r/37880/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Michael Park


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 592)


Do we want to support the following?

```cpp
using google::protobuf::RepeatedPtrField;

auto parse = 
protobuf::parse>>(array_of_array);
```

I think we might want to disallow it since it seems that protobuf 
definitions cannot express "array of array" (maybe there actually is a way to 
do that?). If we do want to disallow it, we should include the following check 
at the top.

```cpp
{ google::protobuf::Message* message = (T*) NULL; (void) message; }
```



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 601)


`s/proagate/propagate/`



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 604)


Remove this newline.


- Michael Park


On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 3, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

2015-09-03 Thread Michael Park


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


Perhaps a dumb question, but may I ask why you needed to introduce 
`SimpleMessage`?

- Michael Park


On Sept. 3, 2015, 1:33 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37827/
> ---
> 
> (Updated Sept. 3, 2015, 1:33 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
> c56d6a3098293eb3659b3066f10e875927ec3ac3 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
> cfc2803e42284f641879fb24bce1282215c8ea52 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
> a1d4084661345f9367c75f9db61279f032b93e69 
>   3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
> bbd36d39e9588eb8eea6d739451ad3bab029ca08 
> 
> Diff: https://reviews.apache.org/r/37827/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> **NOTE**: Filed 
> [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) to clean up 
> protobuf generation.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Michael Park

> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > 
> >
> > Can you explain why this is a struct rather than a function?
> 
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and 
> overload won't work since we take the same argument. Do you think a comment 
> should be expanded?
> 
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
> 
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
> 
> What about using some C++11 type traits?  (But I'm not sure if this will 
> work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template 
> Try parse(const JSON::Value& value,
> typename std::enable_if google::protobuf::Message>, int>::type = 0) {...}
> ```
> 
> Note: This file uses some similar Boost type traits further up.
> 
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate 
> compile-time checks, and not compile-time dispatch. If we want to keep the 
> same function signature, we should rely on partial specialization. However, 
> we can still add these checks into `Parse<>::operator()` methods.
> 
> Alexander Rukletsov wrote:
> Which boils down to replacing
> ```
> { google::protobuf::Message* message = (T*) NULL; (void) message; }
> ```
> with
> ```
> static_assert(std::is_base_of::value,
>   "T must be a protobuf message");
> ```
> Does it make sense?
> 
> Joseph Wu wrote:
> That part makes sense.  
> 
> And just to make sure, you can't use the same pattern found here?  (Which 
> looks a lot like compile-time dispatch.)
> 
> https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L165-L177

We could use the overloaded functions approach using `enable_if`. But it's not 
as clean, and I'll explain what I mean.

First thing to note is that the types are passed explicitly, rather than being 
deduced from parameters.
They would be deduced and can be handled by overload resolution if, for 
example, we opted for a design where we pass the out-parameter.

```cpp
template 
bool parse(const JSON::Value& value, T* out);  // (1)

template 
bool parse(const JSON::Value& value, google::protobuf::RepeatedPtrField* 
out);  // (2)

google::protobuf::Resource resource;
bool a = parse(value, resource)  // invokes (1)
// Check 'a' to determine success.

google::protobuf::RepeatedPtrField resources;
bool b = parse(value, resources);  // invokes (2)
// Check 'b' to determine success.
```

We dispatch according to the overload resolution rules based on how the second 
paramter gets deduced and matches.

With overloaded functions using `enable_if`, the conditions must be mutually 
exclusive,
otherwise the intersections trigger ambiguous overload errors.
The following is what we would need to make it work for our current case.

First we need a helper to determine whether a given `T` is an instance of 
`google::protobuf::RepeatedPtrField` or not.
This is similar to `std::is_integral`, and other type traits helpers. We need 
this to use as our `enable_if` condition.

```cpp
// Use type deduction and overload resolution rules to help determine whether 
`T` is an instance of
// `google::protobuf::RepeatedPtrField` for some `U`.
template 
struct is_repeated
{
  template 
  static std::false_type impl(U &&);

  template 
  static std::true_type impl(google::protobuf::RepeatedPtrField &&);

  static const bool value = decltype(impl(std::declval()))::value;
}
```

We can then use it to mutually exclusively define our `enable_if` conditions:

```cpp
// T is not an instance of google::protobuf::RepeatedPtrField for any U.
template 
std::enable_if_t::value, Try> parse(const JSON::Value& 
value);

// T is google::protobuf::RepeatedPtrField for some U.
template 
std::enable_if_t::value, Try> parse(const JSON::Value& value);
```

Now, to the not-so-clean parts. The first one is that we don't immediately have 
access to the inner type.
That is, inside the body of:

```cpp
// T is google::protobuf::RepeatedPtrField for some U.
template 
std::enable_if_t::value, Try> parse(const JSON::Value& value);
```

We know that `T` is a `google::protobuf::RepeatedPtrField` for some `U`, but 
there's not a `U` for us to use immediately.
We'll have to introduce another type trait like `get_inner` which returns 
the inner type `U` for us.

The other, and more important is that what we express is fundamentally 
different between partial specializations and
overloaded functions with `enable_if`.

Partial specialization gives us the closest thing to pattern matching in C++.
We specify the patterns and dispatch to the best match available.
We can say

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Michael Park


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



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 559)


Remove this newline.



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 567)


Remove this newline.



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 591)


The `Parse` above uses `value`, rather than `json`. Can we consolidate 
to one? I think `value` would be more fitting, since we name `JSON::Object` as 
`object`, `JSON::Array` as `array`, etc.



3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (line 627)


`s/json/value/` as suggested above?


- Michael Park


On Sept. 3, 2015, 1:32 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 3, 2015, 1:32 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37197: Docker image store.

2015-09-03 Thread Jojy Varghese


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



src/slave/containerizer/provisioners/docker/store.hpp (line 100)


Does this need to be in the hpp file?



src/slave/containerizer/provisioners/docker/store.hpp (line 146)


I would rethink the lifecycle management of this poiner.



src/slave/containerizer/provisioners/docker/store.cpp (line 49)


Ususally if you want to provide a factory class that hands out 
implementations, you use Factory class (commonly known as Factory design 
pattern).



src/slave/containerizer/provisioners/docker/store.cpp (line 139)


magic numbers (7) is not recommended.



src/slave/containerizer/provisioners/docker/store.cpp (line 146)


What is the workflow for creating these directories?



src/slave/containerizer/provisioners/docker/store.cpp (line 238)


const?


- Jojy Varghese


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37197: Docker image store.

2015-09-03 Thread Jojy Varghese


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



src/slave/containerizer/provisioners/docker/store.hpp (line 49)


Is this intended to be an abstract class? If so, why this factory method?



src/slave/containerizer/provisioners/docker/store.hpp (line 51)


What is the ownership model for fetcher pointer. By ownership I mean how is 
the lifecycle of the pointer managed?



src/slave/containerizer/provisioners/docker/store.hpp (line 55)


I think all new code needs to follow mesos doxygen guidelines.



src/slave/containerizer/provisioners/docker/store.hpp (line 93)


According to 
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Declaration_Order,
 the non-copyable/non-assignable section should be last in the declaration 
order.



src/slave/containerizer/provisioners/docker/store.cpp (line 150)


calls for a static constant?



src/slave/flags.hpp (line 52)


Wondering we should stuff everything in Flags class or if we can use 
inheritance to make  feature specfic flags.


- Jojy Varghese


On Aug. 27, 2015, 11:39 p.m., Lily Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37197/
> ---
> 
> (Updated Aug. 27, 2015, 11:39 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Jojy Varghese, Till Toenshoff, 
> and Timothy Chen.
> 
> 
> Bugs: MESOS-2849
> https://issues.apache.org/jira/browse/MESOS-2849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stored images currently kept indefinitely.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.cpp PRE-CREATION 
>   src/slave/flags.hpp e56738e2dfd6593ef8f093687919da287af78f77 
>   src/slave/flags.cpp b36710d6d7a7250bc071a57310a2d54bfb3bc624 
> 
> Diff: https://reviews.apache.org/r/37197/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Lily Chen
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Joseph Wu



> On Sept. 1, 2015, 9:46 a.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > 
> >
> > Can you explain why this is a struct rather than a function?
> 
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and 
> overload won't work since we take the same argument. Do you think a comment 
> should be expanded?
> 
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
> 
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
> 
> What about using some C++11 type traits?  (But I'm not sure if this will 
> work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template 
> Try parse(const JSON::Value& value,
> typename std::enable_if google::protobuf::Message>, int>::type = 0) {...}
> ```
> 
> Note: This file uses some similar Boost type traits further up.
> 
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate 
> compile-time checks, and not compile-time dispatch. If we want to keep the 
> same function signature, we should rely on partial specialization. However, 
> we can still add these checks into `Parse<>::operator()` methods.
> 
> Alexander Rukletsov wrote:
> Which boils down to replacing
> ```
> { google::protobuf::Message* message = (T*) NULL; (void) message; }
> ```
> with
> ```
> static_assert(std::is_base_of::value,
>   "T must be a protobuf message");
> ```
> Does it make sense?

That part makes sense.  

And just to make sure, you can't use the same pattern found here?  (Which looks 
a lot like compile-time dispatch.)
https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp#L165-L177


- Joseph


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


On Sept. 3, 2015, 6:32 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 3, 2015, 6:32 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37873: Add quiesce logic in allocator

2015-09-03 Thread Alexander Rukletsov



> On Aug. 30, 2015, 5:10 p.m., Robert Lacroix wrote:
> > src/master/allocator/mesos/hierarchical.hpp, line 153
> > 
> >
> > Are we quiescing resources or rather offers? I think it should be 
> > called `quiesceOffers` so it's symmetrical to `reviveOffers`.

Technically, allocator does not know anything about offers, only about 
allocations. This may change soon (we think about pulling offers management 
into allocator), but until then I find this naming misleading.


- Alexander


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


On Aug. 31, 2015, 5:49 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37873/
> ---
> 
> (Updated Aug. 31, 2015, 5:49 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-3037
> https://issues.apache.org/jira/browse/MESOS-3037
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add quiesce logic in allocator
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 659f37b3f9d9fa02da9bdb6c85cd3c180a24b73a 
>   src/master/allocator/mesos/allocator.hpp 
> aa55755a9c3250579e9366bdbc17a2449e95d659 
>   src/master/allocator/mesos/hierarchical.hpp 
> 38f8fd2c84314bb3731684d0e9795cb4f50a227e 
>   src/master/master.cpp 95207d24db0aa052eb70c4cc7eb75d0611c365cf 
>   src/tests/mesos.hpp b2160f50db3e0b0d04b0625e7589004016c0f746 
> 
> Diff: https://reviews.apache.org/r/37873/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37699: Removed remnants of LIBPROCESS_STATISTICS_WINDOW.

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37699]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 3:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37699/
> ---
> 
> (Updated Sept. 3, 2015, 3:08 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3304
> https://issues.apache.org/jira/browse/MESOS-3304
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed remnants of LIBPROCESS_STATISTICS_WINDOW.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 755187c8761137cb2bf2f7295b29a63f63c68bc6 
> 
> Diff: https://reviews.apache.org/r/37699/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread Jan Schlicht


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

Ship it!



src/tests/containerizer/docker_tests.cpp (line 229)


This is a test of the special case with a "1.7.1.fc22" version string. How 
about an additional test of the general case like "1.7.1"?


- Jan Schlicht


On Sept. 3, 2015, 1:16 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 3, 2015, 1:16 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Vinod Kone


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


looks good overall. some minor issues.


src/master/master.cpp (line 146)


"Remove this handler in 0.26.0".



src/master/master.cpp (lines 149 - 150)


this fits on one line (within 80 chars)? why wrap it?



src/master/master.cpp (lines 181 - 189)


any reason to not just call pong() here?



src/tests/partition_tests.cpp (line 79)


why the change to process::Message here and everywhere else?


- Vinod Kone


On Sept. 3, 2015, 7:46 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38050/
> ---
> 
> (Updated Sept. 3, 2015, 7:46 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1831
> https://issues.apache.org/jira/browse/MESOS-1831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master should send PingSlaveMessage instead of "PING"; Slave should accept 
> PingSlaveMessage but not "PING" message;
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
>   src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 
> 
> Diff: https://reviews.apache.org/r/38050/diff/
> 
> 
> Testing
> ---
> 
> Test2: Start master and slave to debug the PingSlaveMessage. Passed!
> Test1: Run make check successfully!
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 37699: Removed remnants of LIBPROCESS_STATISTICS_WINDOW.

2015-09-03 Thread Greg Mann


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

(Updated Sept. 3, 2015, 3:08 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Removed remnants of LIBPROCESS_STATISTICS_WINDOW.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6 

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


Testing
---

make check


Thanks,

Greg Mann

Re: Review Request 38102: MESOS-3046

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38102]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 2:01 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38102/
> ---
> 
> (Updated Sept. 3, 2015, 2:01 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-3046
> https://issues.apache.org/jira/browse/MESOS-3046
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to 
> UUID::random)
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp 
> e90dabb0c572923a50490ecb17867dc50c6d161d 
> 
> Diff: https://reviews.apache.org/r/38102/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 37505: Fix broken health check in docker executor.

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37505]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 1:25 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Sept. 3, 2015, 1:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
>   src/slave/containerizer/docker.cpp 289d4ec0fba9071dfe0cbf5391b5391d4566dd9c 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38102: MESOS-3046

2015-09-03 Thread Klaus Ma


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

(Updated Sept. 3, 2015, 2:01 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to 
UUID::random)


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp 
e90dabb0c572923a50490ecb17867dc50c6d161d 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Review Request 38102: MESOS-3046

2015-09-03 Thread Klaus Ma


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

Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

MESOS-3046 (Stout's UUID re-seeds a new random generator during each call to 
UUID::random)


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/uuid.hpp 
e90dabb0c572923a50490ecb17867dc50c6d161d 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 37827: Added a test for converting JSON arrays to repeated protobufs.

2015-09-03 Thread Alexander Rukletsov


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

(Updated Sept. 3, 2015, 1:33 p.m.)


Review request for mesos, haosdent huang, Joseph Wu, and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.cpp 
c56d6a3098293eb3659b3066f10e875927ec3ac3 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.h 
cfc2803e42284f641879fb24bce1282215c8ea52 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.pb.cc 
a1d4084661345f9367c75f9db61279f032b93e69 
  3rdparty/libprocess/3rdparty/stout/tests/protobuf_tests.proto 
bbd36d39e9588eb8eea6d739451ad3bab029ca08 

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


Testing
---

make check (Mac OS 10.10.4)

**NOTE**: Filed [MESOS-3323](https://issues.apache.org/jira/browse/MESOS-3323) 
to clean up protobuf generation.


Thanks,

Alexander Rukletsov

Re: Review Request 37830: Added a test for converting JSON arrays to Resources.

2015-09-03 Thread Alexander Rukletsov


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

(Updated Sept. 3, 2015, 1:33 p.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Alexander Rukletsov


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

(Updated Sept. 3, 2015, 1:32 p.m.)


Review request for mesos, Joseph Wu and Michael Park.


Changes
---

Expanded a comment.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
57d5fdf45273c620655b44b5f5572290cffa4bf6 

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


Testing
---

make check (Mac OS 10.10.4)


Thanks,

Alexander Rukletsov

Re: Review Request 37505: Fix broken health check in docker executor.

2015-09-03 Thread haosdent huang


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

(Updated Sept. 3, 2015, 1:25 p.m.)


Review request for mesos, Adam B and Timothy Chen.


Changes
---

Rebase code and update according @tnachen reviews.


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


Repository: mesos


Description
---

Fix broken health check in docker executor.


Diffs (updated)
-

  src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
  src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
  src/docker/executor.cpp 6647075b55e5a79264e3556bb906a1f26a2d673e 
  src/slave/containerizer/docker.cpp 289d4ec0fba9071dfe0cbf5391b5391d4566dd9c 
  src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 

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


Testing
---

# Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
# Docker health check command is run through "docker exec"
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
sudo ./bin/mesos-tests.sh 
--gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" --verbose


Thanks,

haosdent huang

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Alexander Rukletsov



> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > 
> >
> > Can you explain why this is a struct rather than a function?
> 
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and 
> overload won't work since we take the same argument. Do you think a comment 
> should be expanded?
> 
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
> 
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
> 
> What about using some C++11 type traits?  (But I'm not sure if this will 
> work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template 
> Try parse(const JSON::Value& value,
> typename std::enable_if google::protobuf::Message>, int>::type = 0) {...}
> ```
> 
> Note: This file uses some similar Boost type traits further up.
> 
> Alexander Rukletsov wrote:
> I'm not sure how we can use type traits here. IIUC type traits facilitate 
> compile-time checks, and not compile-time dispatch. If we want to keep the 
> same function signature, we should rely on partial specialization. However, 
> we can still add these checks into `Parse<>::operator()` methods.

Which boils down to replacing
```
{ google::protobuf::Message* message = (T*) NULL; (void) message; }
```
with
```
static_assert(std::is_base_of::value,
  "T must be a protobuf message");
```
Does it make sense?


- Alexander


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


On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 1, 2015, 2:20 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37505: Fix broken health check in docker executor.

2015-09-03 Thread haosdent huang



> On Aug. 31, 2015, 4:32 a.m., Timothy Chen wrote:
> > src/docker/executor.cpp, line 361
> > 
> >
> > You should be able to push the arguments with another vector insert?
> > 
> > e.g:
> > 
> > argv.push_back("--executor");
> > argv.push_back(stringify(self());
> > .

If I use this way, I got error like this:
```
Failed to load non-boolean flag 'executor': Missing value

Usage: lt-mesos-health-check [options]

  --executor=VALUE  Executor UPID to send health check messages to
  --health_check_json=VALUE JSON describing health check to perform
  --[no-]help   Prints this help message (default: false)
  --task_id=VALUE   Task ID that this health check process is 
checking
```


- haosdent


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


On Aug. 30, 2015, 10:23 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37505/
> ---
> 
> (Updated Aug. 30, 2015, 10:23 a.m.)
> 
> 
> Review request for mesos, Adam B and Timothy Chen.
> 
> 
> Bugs: MESOS-3136
> https://issues.apache.org/jira/browse/MESOS-3136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix broken health check in docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 6086710fff32a25e46197a69ae1063074317221b 
>   src/docker/docker.cpp 12dc0505c9ec4bd380e817d44da2c4e8d1b0d5f5 
>   src/docker/executor.cpp 256d53d59d5cda63bbeb8c987ce0019e24b9fb77 
>   src/slave/containerizer/docker.cpp a17e4f21e7f5a1dfd47699ec84c7a48fd82294ad 
>   src/tests/health_check_tests.cpp 157a56aa06677d8b7a2cef53b29ed05cb4b5d8ea 
> 
> Diff: https://reviews.apache.org/r/37505/diff/
> 
> 
> Testing
> ---
> 
> # Add two new test cases, HealthCheckTest.ROOT_DOCKER_DockerHealthyTask and 
> HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange
> # Docker health check command is run through "docker exec"
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthyTask" --verbose
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="HealthCheckTest.ROOT_DOCKER_DockerHealthStatusChange" 
> --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37826: Introduced conversion of JSON arrays to repeated protobufs.

2015-09-03 Thread Alexander Rukletsov



> On Sept. 1, 2015, 4:46 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp, lines 553-555
> > 
> >
> > Can you explain why this is a struct rather than a function?
> 
> Alexander Rukletsov wrote:
> It's because we cannot partially specialize function templates and 
> overload won't work since we take the same argument. Do you think a comment 
> should be expanded?
> 
> Alexander Rukletsov wrote:
> http://www.gotw.ca/publications/mill17.htm
> 
> Joseph Wu wrote:
> Yes, I think a comment would be good regardless of how it ends up.
> 
> What about using some C++11 type traits?  (But I'm not sure if this will 
> work.)
> Something like:
> ```
> // Specialization for non-repeated fields.
> template 
> Try parse(const JSON::Value& value,
> typename std::enable_if google::protobuf::Message>, int>::type = 0) {...}
> ```
> 
> Note: This file uses some similar Boost type traits further up.

I'm not sure how we can use type traits here. IIUC type traits facilitate 
compile-time checks, and not compile-time dispatch. If we want to keep the same 
function signature, we should rely on partial specialization. However, we can 
still add these checks into `Parse<>::operator()` methods.


- Alexander


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


On Sept. 1, 2015, 2:20 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37826/
> ---
> 
> (Updated Sept. 1, 2015, 2:20 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/37826/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [37669]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 11:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 3, 2015, 11:16 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread haosdent huang


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

(Updated Sept. 3, 2015, 11:16 a.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.


Changes
---

Update according Bernd opinions.


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


Repository: mesos


Description
---

Ignore overflow docker components in version parsing.


Diffs (updated)
-

  src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
  src/tests/containerizer/docker_tests.cpp 
cfad36850b40ade7a41f5b92255320ca1ed1bf93 

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


Testing
---

# Add new test case
sudo ./bin/mesos-tests.sh 
--gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose


Thanks,

haosdent huang

Re: Review Request 37669: Ignore overflow docker components in version parsing.

2015-09-03 Thread Bernd Mathiske


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

Ship it!


Minor fixes can be dealt with when committing.


src/docker/docker.cpp (line 201)


s/docker/Docker
s/is/does



src/docker/docker.cpp (line 204)


s/to/to a


- Bernd Mathiske


On Sept. 2, 2015, 8:12 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37669/
> ---
> 
> (Updated Sept. 2, 2015, 8:12 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, and Timothy Chen.
> 
> 
> Bugs: MESOS-2986
> https://issues.apache.org/jira/browse/MESOS-2986
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignore overflow docker components in version parsing.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 2e17cedb3e29548bdd2d10bd87bd0f61bf123be8 
>   src/tests/containerizer/docker_tests.cpp 
> cfad36850b40ade7a41f5b92255320ca1ed1bf93 
> 
> Diff: https://reviews.apache.org/r/37669/diff/
> 
> 
> Testing
> ---
> 
> # Add new test case
> sudo ./bin/mesos-tests.sh 
> --gtest_filter="DockerTest.ROOT_DOCKER_parsing_version" --verbose
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Mesos ReviewBot


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


Patch looks great!

Reviews applied: [38050]

All tests passed.

- Mesos ReviewBot


On Sept. 3, 2015, 7:46 a.m., Yong Qiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38050/
> ---
> 
> (Updated Sept. 3, 2015, 7:46 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-1831
> https://issues.apache.org/jira/browse/MESOS-1831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Master should send PingSlaveMessage instead of "PING"; Slave should accept 
> PingSlaveMessage but not "PING" message;
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
>   src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
>   src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 
> 
> Diff: https://reviews.apache.org/r/38050/diff/
> 
> 
> Testing
> ---
> 
> Test2: Start master and slave to debug the PingSlaveMessage. Passed!
> Test1: Run make check successfully!
> 
> 
> Thanks,
> 
> Yong Qiao Wang
> 
>

Re: Review Request 38000: WIP: Added an API for libprocess users to interact with http::AuthenticatorManager

2015-09-03 Thread Alexander Rojas


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

(Updated Sept. 3, 2015, 10:40 a.m.)


Review request for mesos and Benjamin Hindman.


Changes
---

Adding using clause.


Repository: mesos


Description
---

WIP

Adds functions which allows libprocess users to add handlers which require 
authentication as well as functions to install and remove authenticators.

Includes tests.


Diffs (updated)
-

  3rdparty/libprocess/include/process/process.hpp 
009f7c4167fa379ac6b1c267e1b4d5fcdf28d697 
  3rdparty/libprocess/src/process.cpp 755187c8761137cb2bf2f7295b29a63f63c68bc6 
  3rdparty/libprocess/src/tests/http_tests.cpp 
d0b9399d38fa284466a012a21080b1d9007af98b 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Yong Qiao Wang


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

(Updated 九月 3, 2015, 7:46 a.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Master should send PingSlaveMessage instead of "PING"; Slave should accept 
PingSlaveMessage but not "PING" message;


Diffs
-

  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
  src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
  src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 

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


Testing
---

Test2: Start master and slave to debug the PingSlaveMessage. Passed!
Test1: Run make check successfully!


Thanks,

Yong Qiao Wang

Re: Review Request 38050: Master should send PingSlaveMessage instead of "PING"; Slave should accept PingSlaveMessage but not "PING" message;

2015-09-03 Thread Yong Qiao Wang


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

(Updated 九月 3, 2015, 7:45 a.m.)


Review request for mesos and Vinod Kone.


Bugs: MESOS-1831 and MESOS-1832
https://issues.apache.org/jira/browse/MESOS-1831
https://issues.apache.org/jira/browse/MESOS-1832


Repository: mesos


Description
---

Master should send PingSlaveMessage instead of "PING"; Slave should accept 
PingSlaveMessage but not "PING" message;


Diffs (updated)
-

  src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
  src/tests/partition_tests.cpp b7030adcfb1bfb128040aff20f12461412b5382c 
  src/tests/slave_recovery_tests.cpp 4d137e0f1278fdacf71f101b1967df35bfbcdd23 
  src/tests/slave_tests.cpp 24119183ef04961fe2fdac73de7672cbe5b2408c 

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


Testing (updated)
---

Test2: Start master and slave to debug the PingSlaveMessage. Passed!
Test1: Run make check successfully!


Thanks,

Yong Qiao Wang

52 matches

Mail list logo