Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 3, 2015, 6:29 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs (updated) - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Review Request 32820: Fixed the non-POD global variable in perf sampler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/ --- Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Fixed the non-POD global variable in perf sampler. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32820/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 30962: Enabled environment decorator to override.
On March 30, 2015, 3:28 a.m., Adam B wrote: src/hook/manager.cpp, line 130 https://reviews.apache.org/r/30962/diff/5/?file=894741#file894741line130 And if (result.isNone()), is that really supposed to mean that this hook didn't want to modify the env, so the HookManager can leave the environment as is and move onto the next hook? If so, it's probably worth a comment, if not a LOG(INFO). Will leave a comment for now :) - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30962/#review78191 --- On March 13, 2015, 4:04 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30962/ --- (Updated March 13, 2015, 4:04 p.m.) Review request for mesos, Ben Mahler and Kapil Arya. Bugs: MESOS-2351 https://issues.apache.org/jira/browse/MESOS-2351 Repository: mesos Description --- See summary Diffs - src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f Diff: https://reviews.apache.org/r/30962/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/#review78816 --- Bad patch! Reviews applied: [32654] Failed command: ./support/apply-review.sh -n -r 32654 Error: 2015-04-03 18:45:36 URL:https://reviews.apache.org/r/32654/diff/raw/ [1134/1134] - 32654.patch [1] error: patch failed: src/tests/port_mapping_tests.cpp:148 error: src/tests/port_mapping_tests.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On April 3, 2015, 6:29 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 3, 2015, 6:29 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs - src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32805: Terminated the perf subprocess once the parent exits.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/ --- (Updated April 3, 2015, 5:11 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Terminated the perf subprocess once the parent exits. The idea is to have a nanny process which installs a SIGTERM handler which will kill the process group (of course, we need to put the nanny and perf process to the same process group). We set the death signal of the nanny process to be SIGTERM. In that way, when slave exits, the nanny process will receive a SIGTERM, which will then kill all processes in the process group. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32805/diff/ Testing --- sudo make check I also manually verified it by terminating the slave while perf is in progress. The perf is killed immediately. Thanks, Jie Yu
Re: Review Request 32805: Terminated the perf subprocess once the parent exits.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/#review78801 --- Ship it! src/linux/perf.cpp https://reviews.apache.org/r/32805/#comment127831 Add a comment here ? src/linux/perf.cpp https://reviews.apache.org/r/32805/#comment127833 Definitely needs a comment here on what you are trying to do. src/linux/perf.cpp https://reviews.apache.org/r/32805/#comment127840 Comment here that this child runs the perf command. src/linux/perf.cpp https://reviews.apache.org/r/32805/#comment127842 Comment that the parent blocks until the child (i.e., perf command) exits. src/linux/perf.cpp https://reviews.apache.org/r/32805/#comment127834 Comment here too. - Vinod Kone On April 3, 2015, 5:11 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/ --- (Updated April 3, 2015, 5:11 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Terminated the perf subprocess once the parent exits. The idea is to have a nanny process which installs a SIGTERM handler which will kill the process group (of course, we need to put the nanny and perf process to the same process group). We set the death signal of the nanny process to be SIGTERM. In that way, when slave exits, the nanny process will receive a SIGTERM, which will then kill all processes in the process group. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32805/diff/ Testing --- sudo make check I also manually verified it by terminating the slave while perf is in progress. The perf is killed immediately. Thanks, Jie Yu
Re: Review Request 32805: Terminated the perf subprocess once the parent exits.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/#review78809 --- Patch looks great! Reviews applied: [32820, 32805] All tests passed. - Mesos ReviewBot On April 3, 2015, 5:11 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/ --- (Updated April 3, 2015, 5:11 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Terminated the perf subprocess once the parent exits. The idea is to have a nanny process which installs a SIGTERM handler which will kill the process group (of course, we need to put the nanny and perf process to the same process group). We set the death signal of the nanny process to be SIGTERM. In that way, when slave exits, the nanny process will receive a SIGTERM, which will then kill all processes in the process group. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32805/diff/ Testing --- sudo make check I also manually verified it by terminating the slave while perf is in progress. The perf is killed immediately. Thanks, Jie Yu
Re: Review Request 32797: Kill the executor when docker container is destroyed.
On April 3, 2015, 3:46 p.m., Benjamin Hindman wrote: src/slave/containerizer/docker.cpp, line 1230 https://reviews.apache.org/r/32797/diff/1/?file=914221#file914221line1230 Why kill the executor before doing Docker::stop? Can you comment here why you do it in this order versus the other order and the ramifications that has? This is because we're waiting on the executor to finish (os::reaped(executorPid)) in the container-status future, and if we don't kill the executor first the later container-status call will just hang. I can leave a comment about this too. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/#review78785 --- On April 2, 2015, 11:38 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/ --- (Updated April 2, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Kill the executor when docker container is destroyed. Diffs - src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 Diff: https://reviews.apache.org/r/32797/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 32798: Add test to verify executor clean up in docker containerizer.
On April 3, 2015, 3:38 p.m., Benjamin Hindman wrote: src/tests/docker_containerizer_tests.cpp, lines 2625-2627 https://reviews.apache.org/r/32798/diff/1/?file=914222#file914222line2625 My only question here is how do you know the executor is properly killed and cleaned up? Is this because you know there aren't any more child processes? Is that something you want to check after you call Shutdown()? I.e., os::children(0).get().empty() or something like that? Shutdown does indeed checks for remaining child processes, I've verified this by just running the test and get a exception on shutdown. I'll leave a comment in the end too. - Timothy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32798/#review78783 --- On April 2, 2015, 11:38 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32798/ --- (Updated April 2, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Add test to verify executor clean up in docker containerizer. Diffs - src/tests/docker_containerizer_tests.cpp fdd706a892ee1c8d55a406b3f956d99c076c623b Diff: https://reviews.apache.org/r/32798/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/#review78825 --- Patch looks great! Reviews applied: [32654] All tests passed. - Mesos ReviewBot On April 3, 2015, 7:19 p.m., Paul Brett wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 3, 2015, 7:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs - src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/#review78831 --- Ship it! Ship It! - Timothy Chen On April 3, 2015, 8:43 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- (Updated April 3, 2015, 8:43 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Re: Review Request 32820: Fixed the non-POD global variable in perf sampler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/#review78832 --- Ship it! Ship It! - Ian Downes On April 3, 2015, 10:01 a.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/ --- (Updated April 3, 2015, 10:01 a.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Fixed the non-POD global variable in perf sampler. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32820/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- (Updated April 3, 2015, 3:30 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs (updated) - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Re: Suggestion: Mesos 0.22.1 point release
Hi everyone, I think we have everything for the point release now: https://docs.google.com/a/mesosphere.io/spreadsheets/d/1OzAWNjAL4zKtI-jOJqaQUcDNlnrNik2Dd7dHhwFLKcI/edit#gid=0 We planned on making an RC today. So with that in mind, if you have any urgent issues that needs to go into 0.22.1, please let me know :) If not, we will prepare an RC for test and baking during next week, so we can start a vote (hopefully) mid next week. I will keep you posted. Cheers, Niklas On 31 March 2015 at 11:11, Niklas Nielsen nik...@mesosphere.io wrote: Inlined On 30 March 2015 at 18:47, Dave Lester d...@davelester.org wrote: Hi Niklas, Assuming that you'd like to be release manager for this bugfix release, could you create a JIRA issue to track this and add it to the release planning wiki? https://cwiki.apache.org/confluence/display/MESOS/Mesos+Release+Planning I've already updated the page to reflect the previous release and this discussion thread but it'd be good to make this referenceable and managed in JIRA. Thanks Dave! I added the new ticket to the release wiki. I haven't investigated the particular bug you've identified, but it's worth noting that today Henning was in the IRC channel asking questions about an issue they've experienced with the latest release -- may also be worth tracking down. We are in touch and will file a ticket for the issue today. Thanks, Dave On Mon, Mar 30, 2015, at 06:32 PM, Brenden Matthews wrote: +1 for stability. On Mar 30, 2015 6:26 PM, Benjamin Hindman b...@eecs.berkeley.edu wrote: Obviously a +1, this is a stability fix we should get to our users as soon as possible. On Mon, Mar 30, 2015 at 9:01 PM, Niklas Nielsen nik...@mesosphere.io wrote: Hi all, Joris and Ben H recently located and fixed a resident bug in the state abstraction which caused many crashes in the JVM (mostly in conjunction with Marathon) at scale ( https://issues.apache.org/jira/browse/MESOS-1795) We therefore wanted to suggest doing a point release with this fix alongside any high-impact fixes which may have landed between the 0.22.0 release and now (or if reviewable and landed within a couple of days). It should be a release we can do in one to two weeks; otherwise, we should just wait for 0.23.0 Any thoughts/input? Cheers, Niklas
Re: Review Request 32820: Fixed the non-POD global variable in perf sampler.
On April 3, 2015, 8:34 p.m., Ben Mahler wrote: src/linux/perf.cpp, lines 46-51 https://reviews.apache.org/r/32820/diff/1/?file=914812#file914812line46 char[] Also noticed many of our other constants are not static, we may want to do a sweep? Done. I'll do the sweep next week. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/#review78829 --- On April 3, 2015, 5:01 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/ --- (Updated April 3, 2015, 5:01 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Fixed the non-POD global variable in perf sampler. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32820/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 32656: Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32656/ --- (Updated April 3, 2015, 10:36 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2591 https://issues.apache.org/jira/browse/mesos-2591 Repository: mesos Description --- Refactor statistics helper out of PortMappingIsolatorTest for easier reuse. Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 466cd82e69665af217d61392b739b9bba16e1e13 src/slave/containerizer/isolators/network/port_mapping.cpp ccdc44f465f204f674b859c429ba1a6ada51cd88 src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18 Diff: https://reviews.apache.org/r/32656/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32833: Added os::signals::install to install signal handlers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32833/#review78836 --- Ship it! Ship It! - Vinod Kone On April 3, 2015, 9:28 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32833/ --- (Updated April 3, 2015, 9:28 p.m.) Review request for mesos and Vinod Kone. Repository: mesos Description --- Added os::signals::install to install signal handlers. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os/signals.hpp 30232f50cc72a79acd21499fe7602c9bcd624ff6 Diff: https://reviews.apache.org/r/32833/diff/ Testing --- Tested in the later patch. Thanks, Jie Yu
Re: Review Request 32656: Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32656/ --- (Updated April 3, 2015, 9:55 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Rebased. Caught waitForFileCreation change dropped from 32653 Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor statistics helper out of PortMappingIsolatorTest for easier reuse. Diffs (updated) - src/slave/containerizer/isolators/network/port_mapping.hpp 466cd82e69665af217d61392b739b9bba16e1e13 src/slave/containerizer/isolators/network/port_mapping.cpp ccdc44f465f204f674b859c429ba1a6ada51cd88 src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18 Diff: https://reviews.apache.org/r/32656/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32805: Terminated the perf subprocess once the parent exits.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/ --- (Updated April 3, 2015, 9:29 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Changes --- Review comments. Added more comments. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Terminated the perf subprocess once the parent exits. The idea is to have a nanny process which installs a SIGTERM handler which will kill the process group (of course, we need to put the nanny and perf process to the same process group). We set the death signal of the nanny process to be SIGTERM. In that way, when slave exits, the nanny process will receive a SIGTERM, which will then kill all processes in the process group. Diffs (updated) - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32805/diff/ Testing --- sudo make check I also manually verified it by terminating the slave while perf is in progress. The perf is killed immediately. Thanks, Jie Yu
Re: Review Request 32805: Terminated the perf subprocess once the parent exits.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32805/ --- (Updated April 3, 2015, 9:29 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Vinod Kone. Bugs: MESOS-2462 https://issues.apache.org/jira/browse/MESOS-2462 Repository: mesos Description --- Terminated the perf subprocess once the parent exits. The idea is to have a nanny process which installs a SIGTERM handler which will kill the process group (of course, we need to put the nanny and perf process to the same process group). We set the death signal of the nanny process to be SIGTERM. In that way, when slave exits, the nanny process will receive a SIGTERM, which will then kill all processes in the process group. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32805/diff/ Testing --- sudo make check I also manually verified it by terminating the slave while perf is in progress. The perf is killed immediately. Thanks, Jie Yu
Re: Review Request 32820: Fixed the non-POD global variable in perf sampler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/#review78829 --- Ship it! Feel free to split out the proces namespace change. src/linux/perf.cpp https://reviews.apache.org/r/32820/#comment127885 char[] Also noticed many of our other constants are not static, we may want to do a sweep? - Ben Mahler On April 3, 2015, 5:01 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32820/ --- (Updated April 3, 2015, 5:01 p.m.) Review request for mesos, Ben Mahler and Ian Downes. Repository: mesos Description --- Fixed the non-POD global variable in perf sampler. Diffs - src/linux/perf.cpp cad6c80e2a9608ff02fc2b8976efba52713dd5a8 Diff: https://reviews.apache.org/r/32820/diff/ Testing --- make check Thanks, Jie Yu
Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32655/ --- (Updated April 3, 2015, 9:58 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Rebase Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class Diffs (updated) - src/slave/containerizer/mesos/containerizer.hpp ae61a0fcd19f2ba808624312401f020121baf5d4 src/slave/containerizer/mesos/containerizer.cpp e4136095fca55637864f495098189ab3ad8d8fe7 src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18 Diff: https://reviews.apache.org/r/32655/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32655/ --- (Updated April 3, 2015, 10:36 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs: mesos-2591 https://issues.apache.org/jira/browse/mesos-2591 Repository: mesos Description --- Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class Diffs - src/slave/containerizer/mesos/containerizer.hpp ae61a0fcd19f2ba808624312401f020121baf5d4 src/slave/containerizer/mesos/containerizer.cpp e4136095fca55637864f495098189ab3ad8d8fe7 src/tests/port_mapping_tests.cpp 55a5e69ed818fd0856179026e3deb889236fea77 Diff: https://reviews.apache.org/r/32655/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32654: Clean up HostIPNetwork since every use performs the same extract stringify operation
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32654/ --- (Updated April 3, 2015, 7:19 p.m.) Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang. Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Clean up HostIPNetwork since every use performs the same extract stringify operation Diffs (updated) - src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18 Diff: https://reviews.apache.org/r/32654/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/#review78835 --- Patch looks great! Reviews applied: [32832] All tests passed. - Mesos ReviewBot On April 3, 2015, 8:43 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- (Updated April 3, 2015, 8:43 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Review Request 32833: Added os::signals::install to install signal handlers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32833/ --- Review request for mesos and Vinod Kone. Repository: mesos Description --- Added os::signals::install to install signal handlers. Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/os/signals.hpp 30232f50cc72a79acd21499fe7602c9bcd624ff6 Diff: https://reviews.apache.org/r/32833/diff/ Testing --- Tested in the later patch. Thanks, Jie Yu
Review Request 32834: Modifiy gdb scripts error message to check gdb is installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32834/ --- Review request for mesos, Adam B, Cody Maloney, and Niklas Nielsen. Repository: mesos Description --- Ran into a problem where gdb isn't installed and I see a error message saying generated libtool doesn't support gdb. Changed the error message to ask user to make sure gdb is also installed. Diffs - bin/gdb-mesos-local.sh.in 72cfb68b4ff2ac796aa381cf6c49f6a4b83eb28b bin/gdb-mesos-master.sh.in f00af078bb9b8a6c3689d1ddd0db6efe38614d87 bin/gdb-mesos-slave.sh.in e01325c59ed62eb2e0d6bdf24808fc3f0cd206ab bin/gdb-mesos-tests.sh.in 626fefe7d953bf226e6d5fb84c87a6f3d66f4da9 Diff: https://reviews.apache.org/r/32834/diff/ Testing --- make Thanks, Timothy Chen
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32655/ --- (Updated April 3, 2015, 10:17 p.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Correct indentation Bugs: mesos-2332 https://issues.apache.org/jira/browse/mesos-2332 Repository: mesos Description --- Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class Diffs (updated) - src/slave/containerizer/mesos/containerizer.hpp ae61a0fcd19f2ba808624312401f020121baf5d4 src/slave/containerizer/mesos/containerizer.cpp e4136095fca55637864f495098189ab3ad8d8fe7 src/tests/port_mapping_tests.cpp 55a5e69ed818fd0856179026e3deb889236fea77 Diff: https://reviews.apache.org/r/32655/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32507: Moved REGISTER and REREGISTER out of scheduler Calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32507/ --- (Updated April 3, 2015, 11:47 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOs-1127 https://issues.apache.org/jira/browse/MESOs-1127 Repository: mesos Description --- This is because subscription is done before schedulers can send any calls. Diffs - include/mesos/scheduler.hpp efee2cb5fc9f24e84294ed7e05a25cf8c81c2f1a include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32507/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 32844: Added SUBSCRIBE call and SUBSCRIBED event.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32844/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Instead of REGISTER and REREGISTER we now just have SUBSCRIBE. Similarly, instead of REGISTERED and REREGISTERED there is only SUBSCRIBED. This will simplify a scheduler's registration semantics. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32844/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32508: Added version to the scheduler protobufs.
On March 31, 2015, 11:38 p.m., Ben Mahler wrote: Can we punt on version for the initial HTTP API beta release? I say this because: (1) The protobuf objects seems like the wrong place to place a version. If a backwards-incompatible change occurs, we may not be able to parse the 'Call' or 'Event' anymore! Seems more like the protocol should be versioned (e.g. request path, http headers, etc). (2) AFAICT, we're not doing anything that precludes adding versioning as we continue the work on the HTTP API? It just seems like API versioning is a substantial project in its own right and requires quite a bit of thought that we don't necessarily have the time for now (e.g. what numbers are we putting in here? what do they mean? what do we do when we make a backwards incompatible change? how will we keep N and N-1 protobuf definitions available for support?). Seems like one place where we can reduce scope. Good point. I'm discarding this review too and punt on versioning for now. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32508/#review78433 --- On March 31, 2015, 11:17 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32508/ --- (Updated March 31, 2015, 11:17 p.m.) Review request for mesos, Benjamin Hindman and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- See summary. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32508/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32834: Modifiy gdb scripts error message to check gdb is installed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32834/#review78846 --- Patch looks great! Reviews applied: [32834] All tests passed. - Mesos ReviewBot On April 3, 2015, 10:01 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32834/ --- (Updated April 3, 2015, 10:01 p.m.) Review request for mesos, Adam B, Cody Maloney, and Niklas Nielsen. Repository: mesos Description --- Ran into a problem where gdb isn't installed and I see a error message saying generated libtool doesn't support gdb. Changed the error message to ask user to make sure gdb is also installed. Diffs - bin/gdb-mesos-local.sh.in 72cfb68b4ff2ac796aa381cf6c49f6a4b83eb28b bin/gdb-mesos-master.sh.in f00af078bb9b8a6c3689d1ddd0db6efe38614d87 bin/gdb-mesos-slave.sh.in e01325c59ed62eb2e0d6bdf24808fc3f0cd206ab bin/gdb-mesos-tests.sh.in 626fefe7d953bf226e6d5fb84c87a6f3d66f4da9 Diff: https://reviews.apache.org/r/32834/diff/ Testing --- make Thanks, Timothy Chen
Review Request 32843: Updated KILL to include SlaveID.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32843/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having SlaveID will help us do better reconciliation when the task is not found. Also, updated master to handle Call::Kill. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32843/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32656: Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32656/#review78852
---
src/slave/containerizer/isolators/network/port_mapping.hpp
https://reviews.apache.org/r/32656/#comment127929
A quick grep of the code base indicates we mostly favor const char
foobar[]
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32656/#comment127937
what about just directory and name?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32656/#comment127935
Move where used, but see below.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32656/#comment127936
Please fix any s/ // when changing code.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32656/#comment127931
IMHO this is clearer:
```c++
TryJSON::Object parse = JSON::parse();
if (parse.isError()) {
// Comment on why we return on empty object when parsing fails
return JSON::Object();
}
return parse.get();
```
- Ian Downes
On April 3, 2015, 3:36 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32656/
---
(Updated April 3, 2015, 3:36 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: mesos-2591
https://issues.apache.org/jira/browse/mesos-2591
Repository: mesos
Description
---
Refactor statistics helper out of PortMappingIsolatorTest for easier reuse.
Diffs
-
src/slave/containerizer/isolators/network/port_mapping.hpp
466cd82e69665af217d61392b739b9bba16e1e13
src/slave/containerizer/isolators/network/port_mapping.cpp
ccdc44f465f204f674b859c429ba1a6ada51cd88
src/tests/port_mapping_tests.cpp 58709b5a0ac58f13985dcc4b71250ec41487ff18
Diff: https://reviews.apache.org/r/32656/diff/
Testing
---
make check
Thanks,
Paul Brett
Re: Review Request 32506: Added output stream operators for scheduler Calls and Events.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32506/ --- (Updated April 3, 2015, 11:42 p.m.) Review request for mesos and Ben Mahler. Changes --- updated dependency. NNFR. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- For readable output in logs. Diffs (updated) - include/mesos/type_utils.hpp cdf5864389a72002b538c263d70bcade2bdffa45 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b Diff: https://reviews.apache.org/r/32506/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/#review78851 --- Ship it! Ship It! - Adam B On April 3, 2015, 3:58 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- (Updated April 3, 2015, 3:58 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Re: Review Request 32507: Moved REGISTER and REREGISTER out of scheduler Calls.
On March 31, 2015, 10:51 p.m., Ben Mahler wrote: Instead of having a separate /scheduler/events endpoint with a top level Subscribe protobuf, could we just have one /scheduler/call endpoint and have a SUBSCRIBE Call? I prefer this because: (1) There is a single endpoint / Call interface for the scheduler. (2) We don't have to have the special Subscribe top level protobuf. (3) When we move to HTTP/2, we don't have to change anything, we can just tell people that they are now free to use a *single* connection for all calls. For HTTP/1.1, we tell people that they must make the streaming Subscribe call on a standalone connection. Was the motivation for separating the Subscribe call out to a separate endpoint to try to make the connection management more obvious? People already have to think about connection management regardless of having /scheduler/events as a different path. Are there any major reasons I'm missing for not consolidating on one endpoint? Good point. Discarding this review in favor of putting Subscribe inside Call. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32507/#review78423 --- On March 31, 2015, 12:10 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32507/ --- (Updated March 31, 2015, 12:10 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOs-1127 https://issues.apache.org/jira/browse/MESOs-1127 Repository: mesos Description --- This is because subscription is done before schedulers can send any calls. Diffs - include/mesos/scheduler.hpp efee2cb5fc9f24e84294ed7e05a25cf8c81c2f1a include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32507/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32655/ --- (Updated April 4, 2015, 12:39 a.m.) Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Changes --- Updated to reflect review comments Bugs: mesos-2591 https://issues.apache.org/jira/browse/mesos-2591 Repository: mesos Description --- Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class Diffs (updated) - src/slave/containerizer/mesos/containerizer.hpp ae61a0fcd19f2ba808624312401f020121baf5d4 src/slave/containerizer/mesos/containerizer.cpp e4136095fca55637864f495098189ab3ad8d8fe7 src/tests/port_mapping_tests.cpp 55a5e69ed818fd0856179026e3deb889236fea77 Diff: https://reviews.apache.org/r/32655/diff/ Testing --- make check Thanks, Paul Brett
Re: Review Request 32505: Added SHUTDOWN scheduler call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32505/ --- (Updated April 3, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman and Ben Mahler. Changes --- benm's comments. Bugs: MESOS-1127 and MESOS-330 https://issues.apache.org/jira/browse/MESOS-1127 https://issues.apache.org/jira/browse/MESOS-330 Repository: mesos Description --- This is a new call added to explicitly shutdown an executor. Diffs (updated) - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae src/slave/slave.cpp 0f70ebafb0f5b16c560decc66e22a43a52732d09 src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32505/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32509: Documented the scheduler Event/Call protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32509/ --- (Updated April 3, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Changes --- rebased. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 Diff: https://reviews.apache.org/r/32509/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32845: Renamed UNREGISTER call to UNSUBSCRIBE.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/#review78869 --- Patch looks great! Reviews applied: [32500, 32501, 32502, 32504, 32505, 32843, 32506, 32844, 32845] All tests passed. - Mesos ReviewBot On April 3, 2015, 11:50 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/ --- (Updated April 3, 2015, 11:50 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Renamed UNREGISTER call to UNSUBSCRIBE for consistency. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32845/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32850: Moved cram-md5 authenticatee process definition into implementation file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/#review78876 --- Patch looks great! Reviews applied: [32850] All tests passed. - Mesos ReviewBot On April 4, 2015, 3:22 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/ --- (Updated April 4, 2015, 3:22 a.m.) Review request for mesos, Joris Van Remoortere and switched to 'mcypark'. Bugs: MESOS-2584 https://issues.apache.org/jira/browse/MESOS-2584 Repository: mesos-incubating Description --- Removing the process from the header is much cleaner and also fixes the linked clang 3.4.2 JIRA. Apart from that moving, no code is changed. NOTE: This patch is mostly a proof of concept for validation and further research. There are more tests breaking as described in the JIRA and those might need some care as well, depending on our clang 3.4 support strategy. Diffs - src/Makefile.am 9c01f5d src/authentication/cram_md5/authenticatee.hpp 55fac68 src/authentication/cram_md5/authenticatee.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32850/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 32505: Added SHUTDOWN scheduler call.
On April 1, 2015, 12:20 a.m., Ben Mahler wrote: Modulo comments. I noticed you added SlaveID on Shutdown, can you add it to Kill as well? Yup. Will send a patch for it as well. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 127 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line127 Do you want to put this below KILL? I reordered them all in the very last review of this chain. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 174 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line174 Shuts down a custom executor When the executor done. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 176 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line176 If the done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 180 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line180 transition done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 188 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line188 Should you add a required SlaveID to Kill as well since that's the same API as Shutdown? will do it in a separate review. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: include/mesos/scheduler/scheduler.proto, line 235 https://reviews.apache.org/r/32505/diff/2/?file=910158#file910158line235 Below Kill? Reordered in the last review of this chain. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/master/master.hpp, lines 463-465 https://reviews.apache.org/r/32505/diff/2/?file=910159#file910159line463 Above reconcile if you put it below Kill in the API? see above On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/master/master.cpp, lines 1608-1613 https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line1608 Ditto here. will do in the final review where i reorder everything. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/master/master.cpp, line 3454 https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3454 Shouldn't this TODO be in the slave..? added there too. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/master/master.cpp, lines 3475-3493 https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3475 Ditto about ordering w.r.t. other calls. will do once in the final review. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/master/master.cpp, lines 3481-3485 https://reviews.apache.org/r/32505/diff/2/?file=910160#file910160line3481 Looks like you forgot to put the `return` statement here? :) It would be great if you could re-use `drop` here, either by doing the `Slave*` lookup in `receive`, much like we do the framework lookup and drop if not found, or by adding a `drop` overload. But the former seems cleaner. Added return. Good catch! Not all calls have slave id in them unlike framework id. So looking up Slave* in receive doesn't seem correct? On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/slave/slave.cpp, lines 3433-3437 https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3433 The top two LOG(WARNING)s are wrapped on the next line, but the next two are wrapped at the first line, mind just wrapping them all at the first line? done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/slave/slave.cpp, lines 3451-3452 https://reviews.apache.org/r/32505/diff/2/?file=910164#file910164line3451 Mind putting the space in front of yet instead of at the end of not? Makes it a bit clearer that the spaces are correct IMHO because its quick to scan down a straight line with the eyes as opposed to the jagged end of the lines (and we do this above in this function :)). I remember I discussed wrapping || and conditionals at the front of the lines (for the same reason), but that ship has sailed.. done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, lines 346-349 https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line346 Ditto from my other review, don't you need this above the Mesos construction? no. per my comment in the previous review. On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 384 https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line384 newline here? done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 386 https://reviews.apache.org/r/32505/diff/2/?file=910165#file910165line386 newline here? done On April 1, 2015, 12:20 a.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 407
Re: Review Request 32509: Documented the scheduler Event/Call protobufs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32509/#review78874 --- Patch looks great! Reviews applied: [32500, 32501, 32502, 32504, 32505, 32843, 32506, 32844, 32845, 32509] All tests passed. - Mesos ReviewBot On April 3, 2015, 11:55 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32509/ --- (Updated April 3, 2015, 11:55 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- See summary. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 Diff: https://reviews.apache.org/r/32509/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 32850: Moved cram-md5 authenticatee process definition into implementation file.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32850/ --- Review request for mesos, Joris Van Remoortere and switched to 'mcypark'. Bugs: MESOS-2584 https://issues.apache.org/jira/browse/MESOS-2584 Repository: mesos-incubating Description --- Removing the process from the header is much cleaner and also fixes the linked clang 3.4.2 JIRA. Apart from that moving, no code is changed. NOTE: This patch is mostly a proof of concept for validation and further research. There are more tests breaking as described in the JIRA and those might need some care as well, depending on our clang 3.4 support strategy. Diffs - src/Makefile.am 9c01f5d src/authentication/cram_md5/authenticatee.hpp 55fac68 src/authentication/cram_md5/authenticatee.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32850/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Suggestion: Mesos 0.22.1 point release
Based on input from Vinod and Adam; I will reduce the scope on the point release to focus on MESOS-1795 and MESOS-2583. I will move the other tickets back to 0.23.0 if you don't have any objections - let me know if you have any tickets which were regressions in 0.22.0. Also, this will probably generate some JIRA noise - I apologize in advance. Niklas On 3 April 2015 at 13:52, Niklas Nielsen nik...@mesosphere.io wrote: Hi everyone, I think we have everything for the point release now: https://docs.google.com/a/mesosphere.io/spreadsheets/d/1OzAWNjAL4zKtI-jOJqaQUcDNlnrNik2Dd7dHhwFLKcI/edit#gid=0 We planned on making an RC today. So with that in mind, if you have any urgent issues that needs to go into 0.22.1, please let me know :) If not, we will prepare an RC for test and baking during next week, so we can start a vote (hopefully) mid next week. I will keep you posted. Cheers, Niklas On 31 March 2015 at 11:11, Niklas Nielsen nik...@mesosphere.io wrote: Inlined On 30 March 2015 at 18:47, Dave Lester d...@davelester.org wrote: Hi Niklas, Assuming that you'd like to be release manager for this bugfix release, could you create a JIRA issue to track this and add it to the release planning wiki? https://cwiki.apache.org/confluence/display/MESOS/Mesos+Release+Planning I've already updated the page to reflect the previous release and this discussion thread but it'd be good to make this referenceable and managed in JIRA. Thanks Dave! I added the new ticket to the release wiki. I haven't investigated the particular bug you've identified, but it's worth noting that today Henning was in the IRC channel asking questions about an issue they've experienced with the latest release -- may also be worth tracking down. We are in touch and will file a ticket for the issue today. Thanks, Dave On Mon, Mar 30, 2015, at 06:32 PM, Brenden Matthews wrote: +1 for stability. On Mar 30, 2015 6:26 PM, Benjamin Hindman b...@eecs.berkeley.edu wrote: Obviously a +1, this is a stability fix we should get to our users as soon as possible. On Mon, Mar 30, 2015 at 9:01 PM, Niklas Nielsen nik...@mesosphere.io wrote: Hi all, Joris and Ben H recently located and fixed a resident bug in the state abstraction which caused many crashes in the JVM (mostly in conjunction with Marathon) at scale ( https://issues.apache.org/jira/browse/MESOS-1795) We therefore wanted to suggest doing a point release with this fix alongside any high-impact fixes which may have landed between the 0.22.0 release and now (or if reviewable and landed within a couple of days). It should be a release we can do in one to two weeks; otherwise, we should just wait for 0.23.0 Any thoughts/input? Cheers, Niklas
Re: Review Request 32502: Updated RECONCILE call to always specifiy slave id.
On March 31, 2015, 9:49 p.m., Ben Mahler wrote: There is another nice aspect of requiring SlaveID in Reconcile and in Kill, we can make SlaveID in TaskStatus required. Are you planning to add a TODO for that? Will we be ever be able to make it required? Added a TODO. The only way we can make it required though would be to give advance notice to frameworks (executors) to always set it and then make it required in a future version. On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, lines 241-244 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line241 Shouldn't this come before you create the mesos object? REGISTERED event can only be received after a REGISTER call is sent. Just creating the Mesos object doesn't cause any registration to happen. On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, lines 241-244 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line241 Shouldn't this come before you create the mesos object? Otherwise seems like you may receive the REGISTERED event before you set up this expectation? see above. On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 309 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line309 Do you want to check the reason? good idea. done. On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 279 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line279 newline here? done On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 281 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line281 newline here? done On March 31, 2015, 9:49 p.m., Ben Mahler wrote: src/tests/scheduler_tests.cpp, line 298 https://reviews.apache.org/r/32502/diff/2/?file=910153#file910153line298 newline here? done - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/#review78391 --- On March 31, 2015, 12:07 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated March 31, 2015, 12:07 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having a slave id will help us do better reconciliation. So added it to the 'Reconcile' call. Also updated the master to receive the Reconcile call directly. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32502/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32502: Updated RECONCILE call to always specifiy slave id.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated April 3, 2015, 11:36 p.m.) Review request for mesos and Ben Mahler. Changes --- benm's comments. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Having a slave id will help us do better reconciliation. So added it to the 'Reconcile' call. Also updated the master to receive the Reconcile call directly. Diffs (updated) - include/mesos/mesos.proto 3a8e8bf303e0576c212951f6028af77e54d93537 include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 05be911734b8d70c9fa5f3b4a275e8b610621fc5 src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32502/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32655: Refactor out launchHelper to make is usable outside PortMappingIsolatorTest class
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32655/#review78861
---
src/slave/containerizer/mesos/containerizer.hpp
https://reviews.apache.org/r/32655/#comment127938
We favor `const char foobar[]`.
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32655/#comment127939
directory and name?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32655/#comment127943
Move down to where used. Why change and introduce a variable?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32655/#comment127944
Not yours, but why is `preparation` an `Option`? And why don't we check
it's some?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32655/#comment127942
Why the change from std{in,out,err}?
src/tests/port_mapping_tests.cpp
https://reviews.apache.org/r/32655/#comment127941
keep the newline?
- Ian Downes
On April 3, 2015, 3:36 p.m., Paul Brett wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32655/
---
(Updated April 3, 2015, 3:36 p.m.)
Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang.
Bugs: mesos-2591
https://issues.apache.org/jira/browse/mesos-2591
Repository: mesos
Description
---
Refactor out launchHelper to make is usable outside PortMappingIsolatorTest
class
Diffs
-
src/slave/containerizer/mesos/containerizer.hpp
ae61a0fcd19f2ba808624312401f020121baf5d4
src/slave/containerizer/mesos/containerizer.cpp
e4136095fca55637864f495098189ab3ad8d8fe7
src/tests/port_mapping_tests.cpp 55a5e69ed818fd0856179026e3deb889236fea77
Diff: https://reviews.apache.org/r/32655/diff/
Testing
---
make check
Thanks,
Paul Brett
Review Request 32845: Renamed UNREGISTER call to UNSUBSCRIBE.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32845/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-1127 https://issues.apache.org/jira/browse/MESOS-1127 Repository: mesos Description --- Renamed UNREGISTER call to UNSUBSCRIBE for consistency. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/master/master.cpp 618db68ee4163b06e479cf3413eda4b63c9c5a4b src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657 Diff: https://reviews.apache.org/r/32845/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32832: Added CHANGELOG for 0.22.1
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/#review78854 --- Patch looks great! Reviews applied: [32832] All tests passed. - Mesos ReviewBot On April 3, 2015, 10:58 p.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32832/ --- (Updated April 3, 2015, 10:58 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- Added changelog section for Mesos 0.22.1 Diffs - CHANGELOG efcadfa0f896a50f21f34b84bdcaa61046d8cd4b Diff: https://reviews.apache.org/r/32832/diff/ Testing --- Thanks, Niklas Nielsen
Re: Review Request 32583: Marked RunTaskMessage::framework_id as optional.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32583/ --- (Updated April 3, 2015, 10:05 a.m.) Review request for mesos, Adam B and Niklas Nielsen. Changes --- Addressed Joris's comment. Bugs: MESOS-2558 https://issues.apache.org/jira/browse/MESOS-2558 Repository: mesos Description --- The new preferred location for FrameworkID is FrameworkInfo.id. This patchset achieves this goal by incrementally deprecating other locations for FrameworkID. Here is a plan to deal with the upgrade path: For this release (N), we still keep setting RunTaskMessage::framework_id - this would handle older Slaves with newer Master. - added code to handle it being unset in the Slave (handles older Master with newer Slaves). In the following release (N+1), stop reading/setting RunTaskMessage::framework_id - the previous version would handle the unset case. In release N+2, remove the field altogether: - the previous release is not setting/reading it. Diffs (updated) - src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16 Diff: https://reviews.apache.org/r/32583/diff/ Testing --- make check. TODO: Test for upgrade path. Thanks, Kapil Arya
Re: Review Request 32796: Only update docker container when resources differs.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32796/#review78780 --- Ship it! Ship It! - Benjamin Hindman On April 2, 2015, 11:37 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32796/ --- (Updated April 2, 2015, 11:37 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Only update docker container when resources differs. Also include the executor resources when launching the docker container to avoid updating it again later on. Diffs - src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 Diff: https://reviews.apache.org/r/32796/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 32797: Kill the executor when docker container is destroyed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/#review78781 --- Ship it! Ship It! - Benjamin Hindman On April 2, 2015, 11:38 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/ --- (Updated April 2, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Kill the executor when docker container is destroyed. Diffs - src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 Diff: https://reviews.apache.org/r/32797/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 32798: Add test to verify executor clean up in docker containerizer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32798/#review78783 --- Ship it! src/tests/docker_containerizer_tests.cpp https://reviews.apache.org/r/32798/#comment127812 My only question here is how do you know the executor is properly killed and cleaned up? Is this because you know there aren't any more child processes? Is that something you want to check after you call Shutdown()? I.e., os::children(0).get().empty() or something like that? - Benjamin Hindman On April 2, 2015, 11:38 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32798/ --- (Updated April 2, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Add test to verify executor clean up in docker containerizer. Diffs - src/tests/docker_containerizer_tests.cpp fdd706a892ee1c8d55a406b3f956d99c076c623b Diff: https://reviews.apache.org/r/32798/diff/ Testing --- make check Thanks, Timothy Chen
Re: Review Request 32797: Kill the executor when docker container is destroyed.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/#review78785 --- Ship it! I had another thought after my first review, see below please! src/slave/containerizer/docker.cpp https://reviews.apache.org/r/32797/#comment127813 Why kill the executor before doing Docker::stop? Can you comment here why you do it in this order versus the other order and the ramifications that has? - Benjamin Hindman On April 2, 2015, 11:38 p.m., Timothy Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32797/ --- (Updated April 2, 2015, 11:38 p.m.) Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Till Toenshoff. Bugs: MESOS-2583 https://issues.apache.org/jira/browse/MESOS-2583 Repository: mesos Description --- Kill the executor when docker container is destroyed. Diffs - src/slave/containerizer/docker.hpp b7bf54ac65d6c61622e485ac253513eaac2e4f88 src/slave/containerizer/docker.cpp e83b912c707a3f2687b09a647a9ed248a940ea97 Diff: https://reviews.apache.org/r/32797/diff/ Testing --- make check Thanks, Timothy Chen
