Re: Review Request 32501: Removed REQUEST call from scheduler.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/#review78030 --- Ship it! Ship It! - Alexander Rojas On March 26, 2015, 12:07 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32501/ --- (Updated March 26, 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 --- Removed REQUEST call because it is unused. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32501/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
On Feb. 18, 2015, 11:38 a.m., Niklas Nielsen wrote: Let's get tests wired up before committing this :) Adam B wrote: Sure thing. Adding tests in my subsequent patch where we will pass the master's timeout values on to the slave. Will post that very soon. Ben Mahler wrote: Can you do it in one patch? This patch in isolation looks a bit dangerous per our conversation above. Also, please carefully consider whether your approach will be safe to do in a single version. i.e. What happens when there are old slaves running against a new master? And vice versa. Adam B wrote: Easy enough. Added the second patch to this one. Most of the new changes are in messages.proto, slave.hpp/cpp, and partition_tests.cpp, with a few lines in master.cpp to set the new message fields. Certainly safe to upgrade if the new flags stay at their default value. Also, with new slaves talking to an old master, the master will still use the defaults, hence so will the slaves. But old slaves running against a new master with a longer timeout will try to reregister unnecessarily early, so you may want to guarantee that all/most of the slaves have been upgraded before setting these flags, otherwise a large cluster suddenly waking up from a network split would see a lot of unnecessary reregistration attempts. The old behavior in this scenario was that the slaves would reregister after the same default timeout after the network split, and the master would consider them shutdown after the same timeout. If that last scenario is a problem, maybe the better approach is for each slave to specify its own ping timeout, and then the master can use these values with each slave's SlaveObserver. Then we can just require the master(s) to be upgraded first, as is typical. I'm inclined to think that we shouldn't worry too much about the unlikely scenario of a network split in the middle of a rolling upgrade to 0.23 where longer ping timeouts are simultaneously being applied. Procedure should be: upgrade (most of) the cluster, then restart the master(s) with the new longer ping timeout value. I can add a Note to upgrades.md if you like. How does that sound to you @bmahler ? If there are no objections, I'll rebase the patch and update the configuration and upgrades docs. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/#review72992 --- On Feb. 19, 2015, 12:10 a.m., Adam B wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/ --- (Updated Feb. 19, 2015, 12:10 a.m.) Review request for mesos, Ben Mahler and Niklas Nielsen. Bugs: MESOS-2110 https://issues.apache.org/jira/browse/MESOS-2110 Repository: mesos Description --- Added new --slave_ping_timeout and --max_slave_ping_timeouts flags to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs) and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5). These can be extended if slaves are expected/allowed to be down for longer than a minute or two. Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage. Beware that this affects recovery from network timeouts as well as actual slave node/process failover. Diffs - src/master/constants.hpp ad3fe81 src/master/constants.cpp d3d0f71 src/master/flags.hpp 51a6059 src/master/master.cpp f10a3cf src/messages/messages.proto 58484ae src/slave/constants.hpp 12d6e92 src/slave/constants.cpp 7868bef src/slave/slave.hpp 91dae10 src/slave/slave.cpp aec9525 src/tests/fault_tolerance_tests.cpp efa5c57 src/tests/partition_tests.cpp eb16a58 src/tests/slave_recovery_tests.cpp 8210c52 src/tests/slave_tests.cpp 153d9d6 Diff: https://reviews.apache.org/r/29507/diff/ Testing --- Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts. Ran unit tests with shorter non-default values for ping timeouts. `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave Thanks, Adam B
Re: Review Request 29507: Added Configurable Slave Ping Timeouts
On March 6, 2015, 5:19 a.m., Joerg Schad wrote: src/master/flags.hpp, line 383 https://reviews.apache.org/r/29507/diff/4/?file=868836#file868836line383 Shouldn't this also be added to the documentation (i.e. http://mesos.apache.org/documentation/latest/configuration/)? Excellent point. Will update that in the next patch revision. - Adam --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/#review75484 --- On Feb. 19, 2015, 12:10 a.m., Adam B wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29507/ --- (Updated Feb. 19, 2015, 12:10 a.m.) Review request for mesos, Ben Mahler and Niklas Nielsen. Bugs: MESOS-2110 https://issues.apache.org/jira/browse/MESOS-2110 Repository: mesos Description --- Added new --slave_ping_timeout and --max_slave_ping_timeouts flags to mesos-master to supplement the DEFAULT_SLAVE_PING_TIMEOUT (15secs) and DEFAULT_MAX_SLAVE_PING_TIMEOUTS (5). These can be extended if slaves are expected/allowed to be down for longer than a minute or two. Slave will receive master's ping timeout in SlaveRe[re]gisteredMessage. Beware that this affects recovery from network timeouts as well as actual slave node/process failover. Diffs - src/master/constants.hpp ad3fe81 src/master/constants.cpp d3d0f71 src/master/flags.hpp 51a6059 src/master/master.cpp f10a3cf src/messages/messages.proto 58484ae src/slave/constants.hpp 12d6e92 src/slave/constants.cpp 7868bef src/slave/slave.hpp 91dae10 src/slave/slave.cpp aec9525 src/tests/fault_tolerance_tests.cpp efa5c57 src/tests/partition_tests.cpp eb16a58 src/tests/slave_recovery_tests.cpp 8210c52 src/tests/slave_tests.cpp 153d9d6 Diff: https://reviews.apache.org/r/29507/diff/ Testing --- Manually tested slave failover/shutdown with master using different --slave_ping_timeout and --max_slave_ping_timeouts. Ran unit tests with shorter non-default values for ping timeouts. `make check` with new unit tests: ShortPingTimeoutUnreachableMaster and ShortPingTimeoutUnreachableSlave Thanks, Adam B
Re: Review Request 32543: Document problem and solution encountered in Mesos-2419.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- Great. It reads well and explains the issue. Just a couple of questions for you. docs/slave-recovery.md https://reviews.apache.org/r/32543/#comment126411 How about: Troubleshooting: If tasks are not recovering on slave restart when using systemd Let's not word it as a bug, but as a configuration issue that can be easily resolved. docs/slave-recovery.md https://reviews.apache.org/r/32543/#comment126412 Minor: Maybe only link on [KillMode] rather than [default KillMode] docs/slave-recovery.md https://reviews.apache.org/r/32543/#comment126413 (If the slave does not come back, each executorDriver shuts itself down after $MESOS_RECOVERY_TIMEOUT.) Important question: If an executor is killed, does this systemd mode affect whether its tasks would get killed? docs/upgrades.md https://reviews.apache.org/r/32543/#comment126410 Is there some new behavior in Mesos 0.22 that just caused this issue to start occurring? Or could this have happened with Mesos 0.21 or earlier with the same systemd default setting? If so, this is not an upgrade issue and this note doesn't belong in the upgrades doc. - Adam B On March 26, 2015, 4:53 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 26, 2015, 4:53 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Document problem and solution encountered in Mesos-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 32558: Improve compile time of mesos by splitting flags
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78031 --- LGTM Great effort! On my machine (Apple LLVM version 6.0 (clang-600.0.56)) on the current master this reduced 'make -j8' from real5m8.217s, user 25m56.168s, sys 2m47.850s to real4m28.660s, user 23m0.723s, sys 2m36.788s - Joerg Schad On March 27, 2015, 1:21 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/ --- (Updated March 27, 2015, 1:21 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-292 https://issues.apache.org/jira/browse/MESOS-292 Repository: mesos Description --- Split the mesos master, slave flags into header + source file to improve compile time significantly. Should be no functional changes. Largely copy-paste, with a little reworking to remove unnecessary headers from the .hpp, and order headers a little more reliably (as well as remove duplicate includes) in the .cpp. # Impact of these changes `time make check -j8` Before: make check 2732.93s user 103.89s system 514% cpu 9:11.63 total After: make check 2421.18s user 96.60s system 506% cpu 8:16.67 total 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, Intel i7-4790K. The numbers aren't incredibly stable (Other software running, overclocking enabled, etc). They are a good general measure though of speedup. I do a `make check` rather than just `make` because that is what devs do in a day-to-day flow, and if runtime is significantly impacted, it will show up. Test steps: ``` # Warm cache ../configure --disable-python --disable-java make check # Timed build rm -rf * ../configure --disable-python --disable-java time make check ``` Note: Similar, likely greater improvements in compile time would happen if stout were made to not be header only. Specifically stout/os.hpp. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 src/logging/flags.cpp PRE-CREATION src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd src/master/flags.cpp PRE-CREATION src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef src/slave/flags.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32558/diff/ Testing --- make check on ArchLinux with GCC 4.9.2 make distcheck CentOS 7 Thanks, Cody Maloney
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/#review78035 --- I have to say, given the description and summary of the patch and its contents, I was surprised to see the code did much more than just adding the slave id, it also adds the logic to deal with the reconciliation case in the receive call. Can you please update the Description mentioning that? Other than that, it looks good. - Alexander Rojas On March 26, 2015, 12:08 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32502/ --- (Updated March 26, 2015, 12:08 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. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 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 32504: Removed MasterInfo from REGISTER and REREGISTER scheduler calls.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/#review78038 --- src/examples/low_level_scheduler_libprocess.cpp https://reviews.apache.org/r/32504/#comment126427 I think just saying 'registered' is enough, since it is nothing else it registeres with. src/examples/low_level_scheduler_libprocess.cpp https://reviews.apache.org/r/32504/#comment126428 ditto src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32504/#comment126429 ditto. src/examples/low_level_scheduler_pthread.cpp https://reviews.apache.org/r/32504/#comment126430 ditto. - Alexander Rojas On March 26, 2015, 12:10 a.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32504/ --- (Updated March 26, 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 --- Removed MasterInfo from REGISTER and REREGISTER scheduler calls because they don't provide much value in a HTTP API world because the scheduler knows precisely who it is connecting to. Diffs - include/mesos/scheduler/scheduler.proto 783a63ad1cc0edd86605d638046fb959cb6e97e8 src/examples/low_level_scheduler_libprocess.cpp 63d34eefb60d13fe2b82905c1cec9b762340e997 src/examples/low_level_scheduler_pthread.cpp 6d1f938660c02db75bfcbf7c8de0d941cff1920d src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a Diff: https://reviews.apache.org/r/32504/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 32558: Improve compile time of mesos by splitting flags
On March 27, 2015, 11:43 a.m., Joerg Schad wrote: LGTM Great effort! On my machine (Apple LLVM version 6.0 (clang-600.0.56)) on the current master this reduced 'make -j8' from real5m8.217s, user 25m56.168s, sys 2m47.850s to real4m28.660s, user 23m0.723s, sys 2m36.788s Only point I would mention is that this points to a resolved Jira which is strange. And do we have (or could we open) and Jira for the stout/os.hpp splitting? Would be definitely worth starting a discussion about it. - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78031 --- On March 27, 2015, 1:21 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/ --- (Updated March 27, 2015, 1:21 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-292 https://issues.apache.org/jira/browse/MESOS-292 Repository: mesos Description --- Split the mesos master, slave flags into header + source file to improve compile time significantly. Should be no functional changes. Largely copy-paste, with a little reworking to remove unnecessary headers from the .hpp, and order headers a little more reliably (as well as remove duplicate includes) in the .cpp. # Impact of these changes `time make check -j8` Before: make check 2732.93s user 103.89s system 514% cpu 9:11.63 total After: make check 2421.18s user 96.60s system 506% cpu 8:16.67 total 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, Intel i7-4790K. The numbers aren't incredibly stable (Other software running, overclocking enabled, etc). They are a good general measure though of speedup. I do a `make check` rather than just `make` because that is what devs do in a day-to-day flow, and if runtime is significantly impacted, it will show up. Test steps: ``` # Warm cache ../configure --disable-python --disable-java make check # Timed build rm -rf * ../configure --disable-python --disable-java time make check ``` Note: Similar, likely greater improvements in compile time would happen if stout were made to not be header only. Specifically stout/os.hpp. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 src/logging/flags.cpp PRE-CREATION src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd src/master/flags.cpp PRE-CREATION src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef src/slave/flags.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32558/diff/ Testing --- make check on ArchLinux with GCC 4.9.2 make distcheck CentOS 7 Thanks, Cody Maloney
Re: Review Request 31776: Moved allocator to public headers.
On March 23, 2015, 10:27 p.m., Michael Park wrote: include/mesos/master/allocator.proto, lines 20-21 https://reviews.apache.org/r/31776/diff/4/?file=902995#file902995line20 `s/breaking the backward compatibility/breaking backwards compatibility/` Also this is referring to the fact that we can't communicate with the old master if we pull this message out of `internal`, correct? Correct, namespace is part of the type. On March 23, 2015, 10:27 p.m., Michael Park wrote: src/Makefile.am, line 709 https://reviews.apache.org/r/31776/diff/4/?file=902996#file902996line709 Is this a trailing whitespace? Not exactly sure what red highlight means. No, rather RB's quirk : ) On March 23, 2015, 10:27 p.m., Michael Park wrote: src/local/local.hpp, line 28 https://reviews.apache.org/r/31776/diff/4/?file=902997#file902997line28 This comment looks useless. Can we just get rid of it? Alexander Rojas wrote: +1 Totally agree. However, we have a lot of them in our codebase. I would like to get rid of them, but let's do it in a consistent manner: https://issues.apache.org/jira/browse/MESOS-2564 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/#review77477 --- On March 23, 2015, 2:02 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated March 23, 2015, 2:02 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp 9ac75b1f601e14a3d3d117775f37a4a48b291dc6 src/tests/hierarchical_allocator_tests.cpp 93753d1c04159a04a733927a487eb69505438e32 src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 src/tests/master_authorization_tests.cpp ff706ed6f8537207b30a548b0ce2121c5df71ab9 src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7 src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8 src/tests/mesos.cpp c8f43d21b214e75eaac2870cbdf4f03fd18707d1 src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Changes --- changed summary and description Summary (updated) - Documented problem and solution with slave recovery and systemd settings. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description (updated) --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 30931: Added flags to logs at master and slave startup.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30931/#review78046 --- Ship it! Ship It! - Alexander Rojas On March 26, 2015, 12:20 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30931/ --- (Updated March 26, 2015, 12:20 a.m.) Review request for mesos and Till Toenshoff. Bugs: MESOS-2323 https://issues.apache.org/jira/browse/MESOS-2323 Repository: mesos Description --- Added flags to logs at master and slave startup. Diffs - src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 src/slave/slave.cpp 31ca72463abb4ef6629983391527745bbb8df2df Diff: https://reviews.apache.org/r/30931/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 32105: Added operator to stout.flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32105/#review78044 --- Ship it! 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp https://reviews.apache.org/r/32105/#comment126435 `iostream` is quite a big header, can you change it to `ostream`. I feel it does everything we need. - Alexander Rojas On March 26, 2015, 9:29 a.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32105/ --- (Updated March 26, 2015, 9:29 a.m.) Review request for mesos. Bugs: Mesos-2323 https://issues.apache.org/jira/browse/Mesos-2323 Repository: mesos Description --- see summary Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp aedb6ab30d929b81f55270612e76009bd7850daa Diff: https://reviews.apache.org/r/32105/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 32543: Documented problem and solution with slave recovery and systemd settings.
On March 27, 2015, 9:17 a.m., Adam B wrote: docs/upgrades.md, line 15 https://reviews.apache.org/r/32543/diff/2/?file=907124#file907124line15 Is there some new behavior in Mesos 0.22 that just caused this issue to start occurring? Or could this have happened with Mesos 0.21 or earlier with the same systemd default setting? If so, this is not an upgrade issue and this note doesn't belong in the upgrades doc. It it not related to a release (that is why the issue is described in slave-recovery.md). The idea for including it here as well is to warn people explicitly of this issue when upgrading (where this behavior will likely occur). I can drop it, but personally feel it is quite helpful here. - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/#review78025 --- On March 27, 2015, 2:09 p.m., Joerg Schad wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32543/ --- (Updated March 27, 2015, 2:09 p.m.) Review request for mesos, Alexander Rukletsov and Brenden Matthews. Bugs: Mesos-2555 https://issues.apache.org/jira/browse/Mesos-2555 Repository: mesos Description --- Documented the problem and solution encountered in MESOS-2419. Diffs - docs/slave-recovery.md 4bb4a71c6945bd70121743a1e9209a26906773c1 docs/upgrades.md 2a15694607c079ad95ef6cf7f1490872ab9a5976 Diff: https://reviews.apache.org/r/32543/diff/ Testing --- markdown check Thanks, Joerg Schad
Re: Review Request 32105: Added operator to stout.flags.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32105/ --- (Updated March 27, 2015, 2:49 p.m.) Review request for mesos. Changes --- Adressed Alexnders comment and reduced include (iosteam - ostream) Bugs: Mesos-2323 https://issues.apache.org/jira/browse/Mesos-2323 Repository: mesos Description --- see summary Diffs (updated) - 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp aedb6ab30d929b81f55270612e76009bd7850daa Diff: https://reviews.apache.org/r/32105/diff/ Testing --- make check Thanks, Joerg Schad
Re: Review Request 31266: Added support for allocator modules.
On March 24, 2015, 5:55 a.m., Michael Park wrote: Michael Park wrote: Hm, upon looking at other files in `include/mesos/module/` it looks like the 2 style issues I mentioned below is apparent in all of them. Is there a reason for this? or shoud they be fixed as a batch in a separate review request? Yes, I followed the general pattern for modules here. Let's do a batch cleanup here: https://issues.apache.org/jira/browse/MESOS-2565 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/#review77528 --- On March 23, 2015, 2:03 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated March 23, 2015, 2:03 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Build failed in Jenkins: mesos-reviewbot #4860
See https://builds.apache.org/job/mesos-reviewbot/4860/
--
[...truncated 5566 lines...]
rm -rf scheduler/.libs scheduler/_libs
rm -rf slave/.libs slave/_libs
rm -f tests/common/*.o
rm -f usage/*.o
rm -f usage/*.lo
rm -f watcher/*.o
rm -rf slave/containerizer/.libs slave/containerizer/_libs
rm -f watcher/*.lo
rm -f zookeeper/*.o
rm -f zookeeper/*.lo
rm -rf slave/containerizer/isolators/cgroups/.libs
slave/containerizer/isolators/cgroups/_libs
rm -rf slave/containerizer/isolators/filesystem/.libs
slave/containerizer/isolators/filesystem/_libs
rm -rf slave/containerizer/isolators/namespaces/.libs
slave/containerizer/isolators/namespaces/_libs
rm -rf slave/containerizer/isolators/network/.libs
slave/containerizer/isolators/network/_libs
rm -rf slave/containerizer/isolators/posix/.libs
slave/containerizer/isolators/posix/_libs
rm -rf slave/containerizer/mesos/.libs slave/containerizer/mesos/_libs
rm -rf state/.libs state/_libs
rm -rf usage/.libs usage/_libs
rm -rf watcher/.libs watcher/_libs
rm -rf zookeeper/.libs zookeeper/_libs
rm -rf ./.deps authentication/.deps authentication/cram_md5/.deps
authorizer/.deps cli/.deps common/.deps containerizer/.deps docker/.deps
examples/.deps exec/.deps fetcher/.deps files/.deps health-check/.deps
hook/.deps java/jni/.deps jvm/.deps jvm/org/apache/.deps launcher/.deps
linux/.deps linux/routing/.deps linux/routing/diagnosis/.deps
linux/routing/filter/.deps linux/routing/link/.deps
linux/routing/queueing/.deps local/.deps log/.deps log/tool/.deps logging/.deps
master/.deps master/allocator/sorter/drf/.deps messages/.deps module/.deps
sched/.deps scheduler/.deps slave/.deps slave/containerizer/.deps
slave/containerizer/isolators/cgroups/.deps
slave/containerizer/isolators/filesystem/.deps
slave/containerizer/isolators/namespaces/.deps
slave/containerizer/isolators/network/.deps
slave/containerizer/isolators/posix/.deps slave/containerizer/mesos/.deps
state/.deps tests/.deps tests/common/.deps usage/.deps watcher/.deps
zookeeper/.deps
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/src'
Making distclean in ec2
make[2]: Entering directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -rf .libs _libs
rm -f *.lo
test -z || rm -f
test . = ../../ec2 || test -z || rm -f
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -f config.status config.cache config.log configure.lineno
config.status.lineno
rm -f Makefile
make[1]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm -200 -exec
chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
==
mesos-0.23.0 archives ready for distribution:
mesos-0.23.0.tar.gz
==
real13m14.984s
user73m54.980s
sys 5m52.769s
+ chmod -R +w 3rdparty aclocal.m4 ar-lib autom4te.cache bin bootstrap CHANGELOG
compile config.guess config.log config.lt config.status config.sub configure
configure.ac depcomp Dockerfile docs Doxyfile ec2 frameworks include install-sh
libtool LICENSE ltmain.sh m4 Makefile Makefile.am Makefile.in
mesos-0.23.0.tar.gz mesos.pc mesos.pc.in missing mpi NOTICE README.md src
support
+ git clean -fdx
Removing .gitignore
Removing .libs/
Removing .reviewboardrc
Removing 3rdparty/Makefile
Removing 3rdparty/Makefile.in
Removing 3rdparty/libprocess/.deps/
Removing 3rdparty/libprocess/3rdparty/.deps/
Removing 3rdparty/libprocess/3rdparty/Makefile
Removing 3rdparty/libprocess/3rdparty/Makefile.in
Removing 3rdparty/libprocess/3rdparty/gmock_sources.cc
Removing 3rdparty/libprocess/3rdparty/stout/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/aclocal.m4
Removing 3rdparty/libprocess/3rdparty/stout/autom4te.cache/
Removing 3rdparty/libprocess/3rdparty/stout/config.log
Removing 3rdparty/libprocess/3rdparty/stout/config.status
Removing 3rdparty/libprocess/3rdparty/stout/configure
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/missing
Removing 3rdparty/libprocess/Makefile
Removing 3rdparty/libprocess/Makefile.in
Removing 3rdparty/libprocess/aclocal.m4
Removing 3rdparty/libprocess/ar-lib
Removing 3rdparty/libprocess/autom4te.cache/
Removing 3rdparty/libprocess/compile
Removing 3rdparty/libprocess/config.guess
Removing 3rdparty/libprocess/config.log
Removing 3rdparty/libprocess/config.lt
Removing 3rdparty/libprocess/config.status
Removing 3rdparty/libprocess/config.sub
Removing 3rdparty/libprocess/configure
Removing 3rdparty/libprocess/depcomp
Removing 3rdparty/libprocess/include/Makefile
Removing
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 3:08 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Update description. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description (updated) --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Review Request 32579: Fix Attributes and Resources documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/ --- Review request for mesos. Bugs: MESOS-2566 https://issues.apache.org/jira/browse/MESOS-2566 Repository: mesos Description --- See summary. Diffs - docs/attributes-resources.md a65aee3d0792abd7168642c119a041c3b7b97dfc Diff: https://reviews.apache.org/r/32579/diff/ Testing --- Thanks, Michael Park
Re: Review Request 32579: Fix Attributes and Resources documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/#review78083 --- Ship it! Ship It! - Niklas Nielsen On March 27, 2015, 9:54 a.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/ --- (Updated March 27, 2015, 9:54 a.m.) Review request for mesos. Bugs: MESOS-2566 https://issues.apache.org/jira/browse/MESOS-2566 Repository: mesos Description --- See summary. Diffs - docs/attributes-resources.md a65aee3d0792abd7168642c119a041c3b7b97dfc Diff: https://reviews.apache.org/r/32579/diff/ Testing --- Thanks, Michael Park
Re: Review Request 32579: Fix Attributes and Resources documentation.
On March 27, 2015, 5:20 p.m., Joerg Schad wrote: lgtm P.S. checked with Markdown Preview - Joerg --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/#review78076 --- On March 27, 2015, 4:54 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/ --- (Updated March 27, 2015, 4:54 p.m.) Review request for mesos. Bugs: MESOS-2566 https://issues.apache.org/jira/browse/MESOS-2566 Repository: mesos Description --- See summary. Diffs - docs/attributes-resources.md a65aee3d0792abd7168642c119a041c3b7b97dfc Diff: https://reviews.apache.org/r/32579/diff/ Testing --- Thanks, Michael Park
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/#review77511 --- src/master/allocator/mesos/allocator.hpp https://reviews.apache.org/r/31775/#comment125606 You added a JIRA for this, right? Let's kill the comment then. - Niklas Nielsen On March 27, 2015, 9:23 a.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated March 27, 2015, 9:23 a.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp ffccc1259b62db89404f6aa6225beeb4c9828684 src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
On March 24, 2015, 2:49 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, lines 123-126 https://reviews.apache.org/r/31268/diff/4/?file=903030#file903030line123 Could we indent this similar to this example from `src/tests/isolator_tests.cpp`: ``` typedef ::testing::Types CgroupsMemIsolatorProcess, CgroupsCpushareIsolatorProcess, CgroupsPerfEventIsolatorProcess CgroupsIsolatorTypes; ``` ``` typedef ::testing::Types HierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` Alexander Rukletsov wrote: I don't know what is consistent here, `src/tests/cram_md5_authentication_tests.cpp` use the indentation I use. Also, clang-format gives something that is more similar to the way I propose. Yeah I did see the one from `src/tests/cram_md5_authentication_tests.cpp`. I think that one is long enough that we can't do too much better. I would prefer a style that visually separates the list of types with the resulting type. `clang-format` seems to format it as: ``` typedef ::testing::TypesHierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` which also visually separates the list of types and the resulting type so I'm ok with that also. Just explaining exactly what I was pushing for. Having said that, do whatever you feel is right here :) - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review77572 --- On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Jenkins build is back to normal : mesos-reviewbot #4861
See https://builds.apache.org/job/mesos-reviewbot/4861/
Re: Review Request 31265: Provided a factory for allocator in tests.
On March 24, 2015, 2:41 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, line 97 https://reviews.apache.org/r/31265/diff/4/?file=903029#file903029line97 1. Do we need this to be `virtual` right now? 2. `s/createAllocatorInstance/CreateAllocator/` 3. Can we move this either above `SetUp` or below `TearDown`? seems weird to have it sitting in the middle. 1. I though it make sense to mark it virtual, but I'll remove it if you think it's cleaner. 2. We use `camelCase` 3. Sure. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/#review77570 --- On March 23, 2015, 2:11 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated March 23, 2015, 2:11 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
On March 24, 2015, 5:55 a.m., Michael Park wrote: Michael Park wrote: Hm, upon looking at other files in `include/mesos/module/` it looks like the 2 style issues I mentioned below is apparent in all of them. Is there a reason for this? or shoud they be fixed as a batch in a separate review request? Alexander Rukletsov wrote: Yes, I followed the general pattern for modules here. Let's do a batch cleanup here: https://issues.apache.org/jira/browse/MESOS-2565 Sweet, feel free to drop the issues :) - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/#review77528 --- On March 23, 2015, 2:03 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated March 23, 2015, 2:03 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review78074 --- Patch looks great! Reviews applied: [31775, 31776, 31266, 31267, 31262, 31263, 31265, 31268] All tests passed. - Mesos ReviewBot On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Jenkins build is back to normal : mesos-reviewbot #4865
See https://builds.apache.org/job/mesos-reviewbot/4865/
Re: Review Request 32579: Fix Attributes and Resources documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/#review78076 --- lgtm - Joerg Schad On March 27, 2015, 4:54 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/ --- (Updated March 27, 2015, 4:54 p.m.) Review request for mesos. Bugs: MESOS-2566 https://issues.apache.org/jira/browse/MESOS-2566 Repository: mesos Description --- See summary. Diffs - docs/attributes-resources.md a65aee3d0792abd7168642c119a041c3b7b97dfc Diff: https://reviews.apache.org/r/32579/diff/ Testing --- Thanks, Michael Park
Re: Review Request 32579: Fix Attributes and Resources documentation.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/#review78087 --- Patch looks great! Reviews applied: [32579] All tests passed. - Mesos ReviewBot On March 27, 2015, 4:54 p.m., Michael Park wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32579/ --- (Updated March 27, 2015, 4:54 p.m.) Review request for mesos. Bugs: MESOS-2566 https://issues.apache.org/jira/browse/MESOS-2566 Repository: mesos Description --- See summary. Diffs - docs/attributes-resources.md a65aee3d0792abd7168642c119a041c3b7b97dfc Diff: https://reviews.apache.org/r/32579/diff/ Testing --- Thanks, Michael Park
Re: Review Request 31267: Added a test allocator module.
On March 24, 2015, 6:07 a.m., Michael Park wrote: src/tests/module.cpp, line 135 https://reviews.apache.org/r/31267/diff/4/?file=903021#file903021line135 Can we take `Modules*` here instead to keep consistency with `addAnonymousModules`, etc? or perhaps a patch before this one to make all of them `Modules`? Good catch! This is a recent change, functions signatures used to have references when I started the patch : ) On March 24, 2015, 6:07 a.m., Michael Park wrote: src/tests/module.cpp, line 141 https://reviews.apache.org/r/31267/diff/4/?file=903021#file903021line141 Looking at the `Isolator` example which has `Cpu` and `Mem` isolators, it seems to me like we want `s/testdrfalloactor/testallocator/` here. Alexander Rojas wrote: No very sure about this issue, as I see it, we can have multiple isolators, but you cannot have more than one allocator (though I may be wrong). In that sense, it makes sense to have a test for each of them. Just wondering how that will work if there are more than one allocator module that needs to be tested. Michael Park wrote: Ah, good point! I'm not sure about it either. Alex, please feel free to drop the issue. wondering how that will work if there are more than one allocator module that needs to be tested. Typed tests! Yes, I preferred clarity to consistency here. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review77531 --- On March 27, 2015, 3:08 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 3:08 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Build failed in Jenkins: mesos-reviewbot #4863
See https://builds.apache.org/job/mesos-reviewbot/4863/
--
[...truncated 5624 lines...]
rm -f watcher/*.o
rm -f watcher/*.lo
rm -rf slave/containerizer/mesos/.libs slave/containerizer/mesos/_libs
rm -f zookeeper/*.o
rm -f zookeeper/*.lo
rm -rf state/.libs state/_libs
rm -rf usage/.libs usage/_libs
rm -rf watcher/.libs watcher/_libs
rm -rf zookeeper/.libs zookeeper/_libs
rm -rf ./.deps authentication/.deps authentication/cram_md5/.deps
authorizer/.deps cli/.deps common/.deps containerizer/.deps docker/.deps
examples/.deps exec/.deps fetcher/.deps files/.deps health-check/.deps
hook/.deps java/jni/.deps jvm/.deps jvm/org/apache/.deps launcher/.deps
linux/.deps linux/routing/.deps linux/routing/diagnosis/.deps
linux/routing/filter/.deps linux/routing/link/.deps
linux/routing/queueing/.deps local/.deps log/.deps log/tool/.deps logging/.deps
master/.deps master/allocator/sorter/drf/.deps messages/.deps module/.deps
sched/.deps scheduler/.deps slave/.deps slave/containerizer/.deps
slave/containerizer/isolators/cgroups/.deps
slave/containerizer/isolators/filesystem/.deps
slave/containerizer/isolators/namespaces/.deps
slave/containerizer/isolators/network/.deps
slave/containerizer/isolators/posix/.deps slave/containerizer/mesos/.deps
state/.deps tests/.deps tests/common/.deps usage/.deps watcher/.deps
zookeeper/.deps
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/src'
Making distclean in ec2
make[2]: Entering directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -rf .libs _libs
rm -f *.lo
test -z || rm -f
test . = ../../ec2 || test -z || rm -f
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -f config.status config.cache config.log configure.lineno
config.status.lineno
rm -f Makefile
make[1]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm -200 -exec
chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
==
mesos-0.23.0 archives ready for distribution:
mesos-0.23.0.tar.gz
==
real11m28.515s
user64m39.312s
sys 5m7.129s
+ chmod -R +w 3rdparty aclocal.m4 ar-lib autom4te.cache bin bootstrap CHANGELOG
compile config.guess config.log config.lt config.status config.sub configure
configure.ac depcomp Dockerfile docs Doxyfile ec2 frameworks include install-sh
libtool LICENSE ltmain.sh m4 Makefile Makefile.am Makefile.in
mesos-0.23.0.tar.gz mesos.pc mesos.pc.in missing mpi NOTICE README.md src
support
+ git clean -fdx
Removing .gitignore
Removing .libs/
Removing .reviewboardrc
Removing 3rdparty/Makefile
Removing 3rdparty/Makefile.in
Removing 3rdparty/libprocess/.deps/
Removing 3rdparty/libprocess/3rdparty/.deps/
Removing 3rdparty/libprocess/3rdparty/Makefile
Removing 3rdparty/libprocess/3rdparty/Makefile.in
Removing 3rdparty/libprocess/3rdparty/gmock_sources.cc
Removing 3rdparty/libprocess/3rdparty/stout/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/aclocal.m4
Removing 3rdparty/libprocess/3rdparty/stout/autom4te.cache/
Removing 3rdparty/libprocess/3rdparty/stout/config.log
Removing 3rdparty/libprocess/3rdparty/stout/config.status
Removing 3rdparty/libprocess/3rdparty/stout/configure
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/missing
Removing 3rdparty/libprocess/Makefile
Removing 3rdparty/libprocess/Makefile.in
Removing 3rdparty/libprocess/aclocal.m4
Removing 3rdparty/libprocess/ar-lib
Removing 3rdparty/libprocess/autom4te.cache/
Removing 3rdparty/libprocess/compile
Removing 3rdparty/libprocess/config.guess
Removing 3rdparty/libprocess/config.log
Removing 3rdparty/libprocess/config.lt
Removing 3rdparty/libprocess/config.status
Removing 3rdparty/libprocess/config.sub
Removing 3rdparty/libprocess/configure
Removing 3rdparty/libprocess/depcomp
Removing 3rdparty/libprocess/include/Makefile
Removing 3rdparty/libprocess/include/Makefile.in
Removing 3rdparty/libprocess/libtool
Removing 3rdparty/libprocess/ltmain.sh
Removing 3rdparty/libprocess/m4/libtool.m4
Removing 3rdparty/libprocess/m4/ltoptions.m4
Removing 3rdparty/libprocess/m4/ltsugar.m4
Removing 3rdparty/libprocess/m4/ltversion.m4
Removing 3rdparty/libprocess/m4/lt~obsolete.m4
Removing 3rdparty/libprocess/missing
Removing Makefile
Removing Makefile.in
Removing aclocal.m4
Removing ar-lib
Removing autom4te.cache/
Removing bin/gdb-mesos-local.sh
Removing bin/gdb-mesos-master.sh
Removing bin/gdb-mesos-slave.sh
Removing bin/gdb-mesos-tests.sh
Removing bin/lldb-mesos-local.sh
Removing bin/lldb-mesos-master.sh
Removing
Re: Review Request 31265: Provided a factory for allocator in tests.
On March 24, 2015, 2:41 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, line 97 https://reviews.apache.org/r/31265/diff/4/?file=903029#file903029line97 1. Do we need this to be `virtual` right now? 2. `s/createAllocatorInstance/CreateAllocator/` 3. Can we move this either above `SetUp` or below `TearDown`? seems weird to have it sitting in the middle. Alexander Rukletsov wrote: 1. I though it make sense to mark it virtual, but I'll remove it if you think it's cleaner. 2. We use `camelCase` 3. Sure. 1. It wasn't about cleanliness, but rather that I don't think `createAllocator` is a function that should have polymorphic behavior. 2. Seems like we're inconsistent yet again. `src/tests/mesos.hpp` introduces a bunch of non-gtest functions such as `CreateMasterFlags` which are title-cased (I guess to keep consistent with gtest functions). `src/tests/persistent_volumes_tests.cpp` for example kept this pattern and has title-cased functions, but other tests such as `DockerContainerizerTest` in `src/tests/docker_containerizer_tests.cpp` did not. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/#review77570 --- On March 27, 2015, 4:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Build failed in Jenkins: mesos-reviewbot #4864
See https://builds.apache.org/job/mesos-reviewbot/4864/
--
[...truncated 5581 lines...]
rm -rf slave/containerizer/mesos/.libs slave/containerizer/mesos/_libs
rm -rf state/.libs state/_libs
rm -rf usage/.libs usage/_libs
rm -rf watcher/.libs watcher/_libs
rm -rf zookeeper/.libs zookeeper/_libs
rm -rf ./.deps authentication/.deps authentication/cram_md5/.deps
authorizer/.deps cli/.deps common/.deps containerizer/.deps docker/.deps
examples/.deps exec/.deps fetcher/.deps files/.deps health-check/.deps
hook/.deps java/jni/.deps jvm/.deps jvm/org/apache/.deps launcher/.deps
linux/.deps linux/routing/.deps linux/routing/diagnosis/.deps
linux/routing/filter/.deps linux/routing/link/.deps
linux/routing/queueing/.deps local/.deps log/.deps log/tool/.deps logging/.deps
master/.deps master/allocator/sorter/drf/.deps messages/.deps module/.deps
sched/.deps scheduler/.deps slave/.deps slave/containerizer/.deps
slave/containerizer/isolators/cgroups/.deps
slave/containerizer/isolators/filesystem/.deps
slave/containerizer/isolators/namespaces/.deps
slave/containerizer/isolators/network/.deps
slave/containerizer/isolators/posix/.deps slave/containerizer/mesos/.deps
state/.deps tests/.deps tests/common/.deps usage/.deps watcher/.deps
zookeeper/.deps
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/src'
Making distclean in ec2
make[2]: Entering directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -rf .libs _libs
rm -f *.lo
test -z || rm -f
test . = ../../ec2 || test -z || rm -f
rm -f Makefile
make[2]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build/ec2'
rm -f config.status config.cache config.log configure.lineno
config.status.lineno
rm -f Makefile
make[1]: Leaving directory
`https://builds.apache.org/job/mesos-reviewbot/ws/mesos-0.23.0/_build'
if test -d mesos-0.23.0; then find mesos-0.23.0 -type d ! -perm -200 -exec
chmod u+w {} ';' rm -rf mesos-0.23.0 || { sleep 5 rm -rf
mesos-0.23.0; }; else :; fi
==
mesos-0.23.0 archives ready for distribution:
mesos-0.23.0.tar.gz
==
real11m54.844s
user67m58.470s
sys 4m56.063s
+ chmod -R +w 3rdparty aclocal.m4 ar-lib autom4te.cache bin bootstrap CHANGELOG
compile config.guess config.log config.lt config.status config.sub configure
configure.ac depcomp Dockerfile docs Doxyfile ec2 frameworks include install-sh
libtool LICENSE ltmain.sh m4 Makefile Makefile.am Makefile.in
mesos-0.23.0.tar.gz mesos.pc mesos.pc.in missing mpi NOTICE README.md src
support
+ git clean -fdx
Removing .gitignore
Removing .libs/
Removing .reviewboardrc
Removing 3rdparty/Makefile
Removing 3rdparty/Makefile.in
Removing 3rdparty/libprocess/.deps/
Removing 3rdparty/libprocess/3rdparty/.deps/
Removing 3rdparty/libprocess/3rdparty/Makefile
Removing 3rdparty/libprocess/3rdparty/Makefile.in
Removing 3rdparty/libprocess/3rdparty/gmock_sources.cc
Removing 3rdparty/libprocess/3rdparty/stout/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/aclocal.m4
Removing 3rdparty/libprocess/3rdparty/stout/autom4te.cache/
Removing 3rdparty/libprocess/3rdparty/stout/config.log
Removing 3rdparty/libprocess/3rdparty/stout/config.status
Removing 3rdparty/libprocess/3rdparty/stout/configure
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile
Removing 3rdparty/libprocess/3rdparty/stout/include/Makefile.in
Removing 3rdparty/libprocess/3rdparty/stout/missing
Removing 3rdparty/libprocess/Makefile
Removing 3rdparty/libprocess/Makefile.in
Removing 3rdparty/libprocess/aclocal.m4
Removing 3rdparty/libprocess/ar-lib
Removing 3rdparty/libprocess/autom4te.cache/
Removing 3rdparty/libprocess/compile
Removing 3rdparty/libprocess/config.guess
Removing 3rdparty/libprocess/config.log
Removing 3rdparty/libprocess/config.lt
Removing 3rdparty/libprocess/config.status
Removing 3rdparty/libprocess/config.sub
Removing 3rdparty/libprocess/configure
Removing 3rdparty/libprocess/depcomp
Removing 3rdparty/libprocess/include/Makefile
Removing 3rdparty/libprocess/include/Makefile.in
Removing 3rdparty/libprocess/libtool
Removing 3rdparty/libprocess/ltmain.sh
Removing 3rdparty/libprocess/m4/libtool.m4
Removing 3rdparty/libprocess/m4/ltoptions.m4
Removing 3rdparty/libprocess/m4/ltsugar.m4
Removing 3rdparty/libprocess/m4/ltversion.m4
Removing 3rdparty/libprocess/m4/lt~obsolete.m4
Removing 3rdparty/libprocess/missing
Removing Makefile
Removing Makefile.in
Removing aclocal.m4
Removing ar-lib
Removing autom4te.cache/
Removing bin/gdb-mesos-local.sh
Removing bin/gdb-mesos-master.sh
Removing bin/gdb-mesos-slave.sh
Removing bin/gdb-mesos-tests.sh
Removing bin/lldb-mesos-local.sh
Removing bin/lldb-mesos-master.sh
Removing bin/lldb-mesos-slave.sh
Removing bin/lldb-mesos-tests.sh
Removing
Re: Review Request 31776: Moved allocator to public headers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31776/ --- (Updated March 27, 2015, 4:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- This is required for out-of-tree allocator modules. RoleInfo protobuf message has to be extracted into its own public proto file. Diffs (updated) - include/mesos/master/allocator.proto PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82 src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d src/master/master.cpp ffccc1259b62db89404f6aa6225beeb4c9828684 src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8 src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec src/tests/fault_tolerance_tests.cpp a637c32f004638a110390b22cf5b626e904097cf src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/master_authorization_tests.cpp ac79303645cc1af337b1dca8db244113d0ba6fce src/tests/master_slave_reconciliation_tests.cpp e60f601202fcdbb4cafac190e9b09ca6ce925260 src/tests/master_tests.cpp 2bfd0f8a20169649cff03509556b4cfa965a9837 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/mesos.cpp 02cbb4b8cf1206d0f32d160addc91d7e0f1ab28b src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28 Diff: https://reviews.apache.org/r/31776/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31266: Added support for allocator modules.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31266/ --- (Updated March 27, 2015, 4:25 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MPark's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/module/allocator.hpp PRE-CREATION src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/module/manager.cpp 82a38f06e57d034650a6ac32fd73527b38cc97b8 Diff: https://reviews.apache.org/r/31266/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31775: Removed Master::Flags dependency from Allocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31775/ --- (Updated March 27, 2015, 4:23 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- On the way to moving allocator to public includes we need to remove the dependency to internal Master::Flags type. Diffs (updated) - src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/master/allocator/mesos/hierarchical.hpp 9f9a631ee52a3ab194e678f9be139e8dabc7f3db src/master/master.cpp ffccc1259b62db89404f6aa6225beeb4c9828684 src/tests/hierarchical_allocator_tests.cpp 8861bf398e4bb17b0f74eab4f4af26202447ccef src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31775/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 32558: Improve compile time of mesos by splitting flags
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/#review78071 --- src/slave/flags.cpp https://reviews.apache.org/r/32558/#comment126476 How about keeping the same style with namespaces? Just to be consistent with everywhere else in the code base. - Timothy Chen On March 27, 2015, 1:21 a.m., Cody Maloney wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32558/ --- (Updated March 27, 2015, 1:21 a.m.) Review request for mesos, Joris Van Remoortere and Michael Park. Bugs: MESOS-292 https://issues.apache.org/jira/browse/MESOS-292 Repository: mesos Description --- Split the mesos master, slave flags into header + source file to improve compile time significantly. Should be no functional changes. Largely copy-paste, with a little reworking to remove unnecessary headers from the .hpp, and order headers a little more reliably (as well as remove duplicate includes) in the .cpp. # Impact of these changes `time make check -j8` Before: make check 2732.93s user 103.89s system 514% cpu 9:11.63 total After: make check 2421.18s user 96.60s system 506% cpu 8:16.67 total 4 core machine, 8 hypter threads enabled. SSD, 16 GB RAM, Intel i7-4790K. The numbers aren't incredibly stable (Other software running, overclocking enabled, etc). They are a good general measure though of speedup. I do a `make check` rather than just `make` because that is what devs do in a day-to-day flow, and if runtime is significantly impacted, it will show up. Test steps: ``` # Warm cache ../configure --disable-python --disable-java make check # Timed build rm -rf * ../configure --disable-python --disable-java time make check ``` Note: Similar, likely greater improvements in compile time would happen if stout were made to not be header only. Specifically stout/os.hpp. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/logging/flags.hpp 4facb33201cee5a82451a13ca05607c875574752 src/logging/flags.cpp PRE-CREATION src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b src/master/flags.hpp 85ce3285731c11b464aca6eaaa0a9165c73afebd src/master/flags.cpp PRE-CREATION src/slave/flags.hpp 3da71afad38ae41adefab979dbed2ae0b10a98ef src/slave/flags.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32558/diff/ Testing --- make check on ArchLinux with GCC 4.9.2 make distcheck CentOS 7 Thanks, Cody Maloney
Re: Review Request 31776: Moved allocator to public headers.
On March 23, 2015, 10:27 p.m., Michael Park wrote:
include/mesos/master/allocator.proto, lines 20-21
https://reviews.apache.org/r/31776/diff/4/?file=902995#file902995line20
`s/breaking the backward compatibility/breaking backwards
compatibility/`
Also this is referring to the fact that we can't communicate with the
old master if we pull this message out of `internal`, correct?
Alexander Rukletsov wrote:
Correct, namespace is part of the type.
Cool, thanks.
On March 23, 2015, 10:27 p.m., Michael Park wrote:
src/local/local.hpp, line 28
https://reviews.apache.org/r/31776/diff/4/?file=902997#file902997line28
This comment looks useless. Can we just get rid of it?
Alexander Rojas wrote:
+1
Alexander Rukletsov wrote:
Totally agree. However, we have a lot of them in our codebase. I would
like to get rid of them, but let's do it in a consistent manner:
https://issues.apache.org/jira/browse/MESOS-2564
Sounds good to me. Still, staying consistent would look like this:
```
namespace mesos {
namespace internal {
// Forward declarations.
namespace master {
class Master;
} // namespace master {
class Configuration;
namespace local {
// Launch a local cluster with the given flags.
process::PIDmaster::Master launch(
const Flags flags,
mesos::master::allocator::Allocator* _allocator = NULL);
void shutdown();
} // namespace local {
} // namespace internal
} // namespace mesos
```
`Master` and `Configuration` are the forward declarations.
Followed by the declarations being made in the `local` namespace.
The `// Forward declarations.` comment being on top of the `mesos` namespace
would indicate that the whole file is forward declarations.
- Michael
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31776/#review77477
---
On March 23, 2015, 2:02 p.m., Alexander Rukletsov wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31776/
---
(Updated March 23, 2015, 2:02 p.m.)
Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen.
Bugs: MESOS-2160
https://issues.apache.org/jira/browse/MESOS-2160
Repository: mesos
Description
---
This is required for out-of-tree allocator modules. RoleInfo protobuf message
has to be extracted into its own public proto file.
Diffs
-
include/mesos/master/allocator.proto PRE-CREATION
src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb
src/local/local.hpp 0aa50ef4f44c347eca4b3a409f2a8d03ba321d82
src/local/local.cpp 19083368212b24ce1afef3a5f91d48766d1cd55e
src/master/allocator/allocator.hpp b67b8fddbd7a3fffc6fe24d5e77cd1db8cb6f69b
src/master/allocator/mesos/allocator.hpp
fb898f1175b61b442204e6e38c69ccc2838a646f
src/master/main.cpp 7cce3a0bb808a1cb7bac9acab31eb1c67a15ea9f
src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d
src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/tests/cluster.hpp a56b6541adcdebc5866571bbdbb6828df97b34ec
src/tests/fault_tolerance_tests.cpp
9ac75b1f601e14a3d3d117775f37a4a48b291dc6
src/tests/hierarchical_allocator_tests.cpp
93753d1c04159a04a733927a487eb69505438e32
src/tests/master_allocator_tests.cpp
a432d0207e1a92532a495bf9ad2826414ee4f6f0
src/tests/master_authorization_tests.cpp
ff706ed6f8537207b30a548b0ce2121c5df71ab9
src/tests/master_slave_reconciliation_tests.cpp
e60f601202fcdbb4cafac190e9b09ca6ce925260
src/tests/master_tests.cpp e69348be676a80017062e3abbd15b8008a6009d7
src/tests/mesos.hpp 45e35204d1aa876fa0c871acf0f21afcd5ababe8
src/tests/mesos.cpp c8f43d21b214e75eaac2870cbdf4f03fd18707d1
src/tests/rate_limiting_tests.cpp d5c00b8db30fc529bc984417a632cf99eb50eb28
Diff: https://reviews.apache.org/r/31776/diff/
Testing
---
make check (Mac OS 10.9.5, CentOS 7.0)
Thanks,
Alexander Rukletsov
Re: Review Request 31262: Moved allocator actions before TestAllocator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31262/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Once TestAllocator is not a template, the allocator actions should be defined before they are used in TestAllocator. Diffs (updated) - src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 Diff: https://reviews.apache.org/r/31262/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 4:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MParks's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs (updated) - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
On March 24, 2015, 2:49 p.m., Michael Park wrote: src/tests/master_allocator_tests.cpp, lines 123-126 https://reviews.apache.org/r/31268/diff/4/?file=903030#file903030line123 Could we indent this similar to this example from `src/tests/isolator_tests.cpp`: ``` typedef ::testing::Types CgroupsMemIsolatorProcess, CgroupsCpushareIsolatorProcess, CgroupsPerfEventIsolatorProcess CgroupsIsolatorTypes; ``` ``` typedef ::testing::Types HierarchicalDRFAllocator, tests::ModuleAllocator, TestDRFAllocator AllocatorTypes; ``` I don't know what is consistent here, `src/tests/cram_md5_authentication_tests.cpp` use the indentation I use. Also, clang-format gives something that is more similar to the way I propose. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/#review77572 --- On March 23, 2015, 2:27 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 23, 2015, 2:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs - src/tests/master_allocator_tests.cpp a432d0207e1a92532a495bf9ad2826414ee4f6f0 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31268: Wired up test allocator to allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31268/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- Same typed tests are used for built-in and modularized allocators. Diffs (updated) - src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31268/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31263: Refactored TestAllocator and allocator text fixture.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31263/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Rebased. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- TestAllocator stores a pointer to a real allocator and takes over its ownership. MasterAllocatorTest fixture stores the test allocator in turn. Diffs (updated) - src/tests/containerizer.cpp 26b87ac6b16dfeaf84888e80296ef540697bd775 src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 src/tests/mesos.hpp 0e98572a62ae05437bd2bc800c370ad1a0c43751 src/tests/resource_offers_tests.cpp 882a9ff4d09aace486182828bf43b643b0d0c519 src/tests/slave_recovery_tests.cpp 87f4a6aab27d142fa8eb7a6571f684a6ce59687e Diff: https://reviews.apache.org/r/31263/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 31265: Provided a factory for allocator in tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31265/ --- (Updated March 27, 2015, 4:27 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Changes --- Addressed MParks's comments. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The factory creates allocator instances in a way identical to how instances from modules are created. It allows us to use same typed tests for built-in and modularized allocators. Diffs (updated) - src/master/allocator/mesos/allocator.hpp fb898f1175b61b442204e6e38c69ccc2838a646f src/tests/master_allocator_tests.cpp 03a1bb8c92b44bc1ad1b5f5cff8d1fb971df2302 Diff: https://reviews.apache.org/r/31265/diff/ Testing --- make check (Mac OS 10.9.5, Ubuntu 14.04) Thanks, Alexander Rukletsov
Re: Review Request 31267: Added a test allocator module.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/#review78070 --- src/tests/module.cpp https://reviews.apache.org/r/31267/#comment126474 You may need a `CHECK(modules != NULL)` or a log and return in the NULL case? - Alexander Rojas On March 27, 2015, 5:26 p.m., Alexander Rukletsov wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31267/ --- (Updated March 27, 2015, 5:26 p.m.) Review request for mesos, Kapil Arya, Michael Park, and Niklas Nielsen. Bugs: MESOS-2160 https://issues.apache.org/jira/browse/MESOS-2160 Repository: mesos Description --- The test allocator module is a built-in HierarchicalDRFAllocator put in a module. NOTE: Here we add harness code to load the module, tests will be wired up later in the patch stack. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/examples/test_allocator_module.cpp PRE-CREATION src/tests/module.hpp 65e567f21744d9a2b07b8680d45bcd1ea47f9508 src/tests/module.cpp b81144f56e94034feecf3a6a4992af078cf60a81 Diff: https://reviews.apache.org/r/31267/diff/ Testing --- make check (Mac OS 10.9.5, CentOS 7.0) Thanks, Alexander Rukletsov
Re: Review Request 32505: Added SHUTDOWN scheduler call.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/#review78050
---
src/master/master.cpp
https://reviews.apache.org/r/32505/#comment126439
I think the common pattern is to do early return on failure, i.e.:
```cpp
if (framework == NULL) {
return;
}
…
```
src/scheduler/scheduler.cpp
https://reviews.apache.org/r/32505/#comment126449
Do you mind adding a comment why an exited executor is always marked as a
failure? It is not clear to me.
- Alexander Rojas
On March 26, 2015, 12:12 a.m., Vinod Kone wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32505/
---
(Updated March 26, 2015, 12:12 a.m.)
Review request for mesos, Benjamin Hindman and Ben Mahler.
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
-
include/mesos/scheduler/scheduler.proto
783a63ad1cc0edd86605d638046fb959cb6e97e8
src/master/master.hpp 3c957abcb54a0c23b8549c1d21d2d9277791938d
src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1
src/messages/messages.proto 97c45c01dfcea38b1ae555c036d61e10c152c2c8
src/scheduler/scheduler.cpp 584b042e32865fdf875bf41ebcfb7f9c327d882a
src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp c7e65a6c095963feaa9d5fdbb519c68f8f761d16
src/tests/scheduler_tests.cpp 4a89a7a88b50bb8c254f5076661ce07ac9fc7657
Diff: https://reviews.apache.org/r/32505/diff/
Testing
---
make check
Thanks,
Vinod Kone
Splitting reviews and build emails into their own mailing lists
Hi, What do you guys think about moving the review and build (CI) emails to their own mailing lists (reviews@ and builds@). Quite a few people that I have talked to have mentioned that there is too much noise on our dev list to get at the signal (dev discussions, release announcements, API changes etc). We have taken the first step by moving JIRA emails (issues@) off the dev list. I think it's time we move reviews/build emails too to keep the fidelity of dev list high. Comments?
Re: Review Request 31276: Added cgroup memory pressure listening tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/#review78101 --- Ship it! Ship It! - Jie Yu On March 27, 2015, 3:20 a.m., Chi Zhang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31276/ --- (Updated March 27, 2015, 3:20 a.m.) Review request for mesos, Dominic Hamon, Ian Downes, and Jie Yu. Bugs: mesos-2136 https://issues.apache.org/jira/browse/mesos-2136 Repository: mesos Description --- Added cgroup memory pressure listening tests. Diffs - src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/tests/cgroups_tests.cpp 75c61aad80f894acb92a9752e8d1b6af70e5b9a6 src/tests/memory_test_helper.hpp PRE-CREATION src/tests/memory_test_helper.cpp PRE-CREATION src/tests/memory_test_helper_main.cpp PRE-CREATION Diff: https://reviews.apache.org/r/31276/diff/ Testing --- Thanks, Chi Zhang
Re: Review Request 32152: Fix memory corruption in AbstractState JNI bindings. MESOS-2161.
On March 27, 2015, 6:51 p.m., Benjamin Hindman wrote: src/java/jni/org_apache_mesos_state_AbstractState.cpp, line 249 https://reviews.apache.org/r/32152/diff/4/?file=907011#file907011line249 Can we confirm that we can make a jclass be static? I believe this can't be made `static` unless we make it a global like so: ``` static jclass clazz = (jclass)env-NewGlobalRef(env-GetObjectClass(thiz)); ``` I think it's the same situation as: [MESOS-2414](https://issues.apache.org/jira/browse/MESOS-2414). - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/#review78092 --- On March 27, 2015, 12:02 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/ --- (Updated March 27, 2015, 12:02 a.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_state_AbstractState.cpp 1accc8a498a68b7cfd9e39dc1f3ce01c8bfd219f src/java/src/org/apache/mesos/state/AbstractState.java c66bf0519e7fc671d1e167ccd1e778dc65d3d8e6 Diff: https://reviews.apache.org/r/32152/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Splitting reviews and build emails into their own mailing lists
Awesome Vinod, I'm fully on board with this idea. I think the dev discussions in specific sometimes get drowned by the reviews and other noise that's conflated into this mailing list. Thanks for taking the steps on this! MPark. On 27 March 2015 at 15:19, Vinod Kone vinodk...@apache.org wrote: Hi, What do you guys think about moving the review and build (CI) emails to their own mailing lists (reviews@ and builds@). Quite a few people that I have talked to have mentioned that there is too much noise on our dev list to get at the signal (dev discussions, release announcements, API changes etc). We have taken the first step by moving JIRA emails (issues@) off the dev list. I think it's time we move reviews/build emails too to keep the fidelity of dev list high. Comments?
Re: Review Request 32152: Fix memory corruption in AbstractState JNI bindings. MESOS-2161.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/#review78092 --- Ship it! src/java/jni/org_apache_mesos_state_AbstractState.cpp https://reviews.apache.org/r/32152/#comment126525 s/.././ src/java/jni/org_apache_mesos_state_AbstractState.cpp https://reviews.apache.org/r/32152/#comment126519 Can we confirm that we can make a jclass be static? src/java/jni/org_apache_mesos_state_AbstractState.cpp https://reviews.apache.org/r/32152/#comment126524 Please add a minor comment above each of these 'return' lines that says something like: // NOTE: See TODO at top of file for why we proxy. src/java/jni/org_apache_mesos_state_AbstractState.cpp https://reviews.apache.org/r/32152/#comment126523 4 space indent src/java/jni/org_apache_mesos_state_AbstractState.cpp https://reviews.apache.org/r/32152/#comment126517 Kill whitespace please. src/java/src/org/apache/mesos/state/AbstractState.java https://reviews.apache.org/r/32152/#comment126520 Can kill the 'try' once we change MesosNativeLibrary.LibraryNotLoadedException to RuntimeException. Here and below. - Benjamin Hindman On March 27, 2015, 12:02 a.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/ --- (Updated March 27, 2015, 12:02 a.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_state_AbstractState.cpp 1accc8a498a68b7cfd9e39dc1f3ce01c8bfd219f src/java/src/org/apache/mesos/state/AbstractState.java c66bf0519e7fc671d1e167ccd1e778dc65d3d8e6 Diff: https://reviews.apache.org/r/32152/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 32151: Add MESOS_{MAJOR|MINOR|PATCH}_VERSION to libmesos.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32151/#review78081
---
Ship it!
src/common/version.cpp
https://reviews.apache.org/r/32151/#comment126502
Let's include:
failed to parse Mesos version ' MESOS_VERSION ';
Here and below please!
src/common/version.cpp
https://reviews.apache.org/r/32151/#comment126508
Let's just:
return version-get().major;
Here and below please.
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126510
This should all be Javadoc comments (/** */).
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126485
s/new/New/
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126486
s/old/Old/
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126496
implements Comparable
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126511
Javadoc comments please, here and everywhere else!
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126497
@Override
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126500
if (other == null) {
throw new IllegalArgumentException(other Version must not be null);
}
if (major other.major) {
return -1;
} else if (major other.major) {
return 1;
}
if (minor other.minor) {
return -1;
} else if (minor other.minor) {
return 1;
}
...
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126509
Let's leave a comment that this is not equal to and before but just
strictly before, similar down below for 'after' as well.
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
https://reviews.apache.org/r/32151/#comment126491
Let's make this a RuntimeException instead. Why? This provides similar
semantics to if we attempted to call a native method and haven't first called
MesosNativeLibrary.load().
- Benjamin Hindman
On March 27, 2015, 12:01 a.m., Joris Van Remoortere wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/32151/
---
(Updated March 27, 2015, 12:01 a.m.)
Review request for mesos, Benjamin Hindman and Niklas Nielsen.
Bugs: MESOS-1795 and MESOS-2161
https://issues.apache.org/jira/browse/MESOS-1795
https://issues.apache.org/jira/browse/MESOS-2161
Repository: mesos
Description
---
See summary.
Diffs
-
include/mesos/version.hpp.in 524cebce5d84edac3ef5d0b72e3989f283164809
src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb
src/common/version.cpp PRE-CREATION
src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in
668647fdfc0e203fcde59263256659ba14e29960
src/java/jni/org_apache_mesos_MesosNativeLibrary.cpp PRE-CREATION
src/tests/common/version_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/32151/diff/
Testing
---
Thanks,
Joris Van Remoortere
Re: Review Request 32151: Add MESOS_{MAJOR|MINOR|PATCH}_VERSION to libmesos.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32151/ --- (Updated March 27, 2015, 10:39 p.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Changes --- -Clean up version macros -Version implements Comparable -Delegate default constructors for Version -Add illegal argument checks for Version constructor -Add some extra comments -Change to javadoc comments where applicable -Change version introduction barrier to 0.22.1 Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/version.hpp.in 524cebce5d84edac3ef5d0b72e3989f283164809 src/Makefile.am 7a06c7028eca8164b1f5fdea6a7ecd37ee6826bb src/common/version.cpp PRE-CREATION src/java/generated/org/apache/mesos/MesosNativeLibrary.java.in 668647fdfc0e203fcde59263256659ba14e29960 src/java/jni/org_apache_mesos_MesosNativeLibrary.cpp PRE-CREATION src/tests/common/version_tests.cpp PRE-CREATION Diff: https://reviews.apache.org/r/32151/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 32152: Fix memory corruption in AbstractState JNI bindings. MESOS-2161.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/ --- (Updated March 27, 2015, 10:43 p.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Changes --- -Add global ref to jclass caching -Fix / Add comments -Change bug fix version to 0.22.1 -Rebase to get rid of LibraryNotLoadedException Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs (updated) - src/java/jni/org_apache_mesos_state_AbstractState.cpp 1accc8a498a68b7cfd9e39dc1f3ce01c8bfd219f src/java/src/org/apache/mesos/state/AbstractState.java c66bf0519e7fc671d1e167ccd1e778dc65d3d8e6 Diff: https://reviews.apache.org/r/32152/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 32152: Fix memory corruption in AbstractState JNI bindings. MESOS-2161.
On March 27, 2015, 6:51 p.m., Benjamin Hindman wrote: src/java/jni/org_apache_mesos_state_AbstractState.cpp, line 249 https://reviews.apache.org/r/32152/diff/4/?file=907011#file907011line249 Can we confirm that we can make a jclass be static? Michael Park wrote: I believe this can't be made `static` unless we make it a global like so: ``` static jclass clazz = (jclass)env-NewGlobalRef(env-GetObjectClass(thiz)); ``` I think it's the same situation as: [MESOS-2414](https://issues.apache.org/jira/browse/MESOS-2414). Indeed we can not. Using Mpark's strategy. - Joris --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/#review78092 --- On March 27, 2015, 10:43 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/ --- (Updated March 27, 2015, 10:43 p.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_state_AbstractState.cpp 1accc8a498a68b7cfd9e39dc1f3ce01c8bfd219f src/java/src/org/apache/mesos/state/AbstractState.java c66bf0519e7fc671d1e167ccd1e778dc65d3d8e6 Diff: https://reviews.apache.org/r/32152/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- Fix looks good to me. Buildbot, please take another look. ;) - Adam B On March 25, 2015, 4:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 4:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 32152: Fix memory corruption in AbstractState JNI bindings. MESOS-2161.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/#review78124 --- Patch looks great! Reviews applied: [32550, 32151, 32152] All tests passed. - Mesos ReviewBot On March 27, 2015, 10:43 p.m., Joris Van Remoortere wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32152/ --- (Updated March 27, 2015, 10:43 p.m.) Review request for mesos, Benjamin Hindman and Niklas Nielsen. Bugs: MESOS-1795 and MESOS-2161 https://issues.apache.org/jira/browse/MESOS-1795 https://issues.apache.org/jira/browse/MESOS-2161 Repository: mesos Description --- See summary. Diffs - src/java/jni/org_apache_mesos_state_AbstractState.cpp 1accc8a498a68b7cfd9e39dc1f3ce01c8bfd219f src/java/src/org/apache/mesos/state/AbstractState.java c66bf0519e7fc671d1e167ccd1e778dc65d3d8e6 Diff: https://reviews.apache.org/r/32152/diff/ Testing --- Thanks, Joris Van Remoortere
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Can you or @till let me know what the race and temporal issue here was? And how this fixes it? - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Vinod Kone wrote: Can you or @till let me know what the race and temporal issue here was? And how this fixes it? There was one major problem in these lines: ``` // Start authentication. const FutureOptionstring future = authenticator.get()-authenticate(from) .onAny(defer(self(), Self::_authenticate, pid, lambda::_1)); ``` The returned future was destructed before the local reference (`future`) would even receive it. On the left side, we have a const ref. On the right side, we have a temporal (returned by `authenticate`). This in itself is fine as the lifetime of the temporary will be extended by the lifetime of the reference. Trouble is that the expression `onAny` then gets called on a reference to a temporary and generates a reference to that on return. The returned reference will not extend the lifetime of the future. Now havoc breaks lose as we have a dangling reference. The problem is not showing on all systems as they OS may not immediately reuse the destroyed objects' memory. For the possible race - that actually was not really a race - my update-comment was misleading. It was the fact that I realized that an onAny/... assignment could immediately trigger a callback invocation. Our defer certainly delays the callback but I felt that it would look much cleaner if I made sure that the initializing of authenticating happened before assigning the callback. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78149 --- Patch looks great! Reviews applied: [27760] All tests passed. - Mesos ReviewBot On March 28, 2015, 2:25 a.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 28, 2015, 2:25 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 28, 2015, 2:25 a.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Changes --- No changes, hoping to trigger review-bot. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs (updated) - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
Re: Review Request 27760: Revised authenticator interface to allow for two fold implementations.
On March 27, 2015, 10:46 p.m., Adam B wrote: Fix looks good to me. Buildbot, please take another look. ;) Vinod Kone wrote: Can you or @till let me know what the race and temporal issue here was? And how this fixes it? Till Toenshoff wrote: There was one major problem in these lines: ``` // Start authentication. const FutureOptionstring future = authenticator.get()-authenticate(from) .onAny(defer(self(), Self::_authenticate, pid, lambda::_1)); ``` The returned future was destructed before the local reference (`future`) would even receive it. On the left side, we have a const ref. On the right side, we have a temporal (returned by `authenticate`). This in itself is fine as the lifetime of the temporary will be extended by the lifetime of the reference. Trouble is that the expression `onAny` then gets called on a reference to a temporary and generates a reference to that on return. The returned reference will not extend the lifetime of the future. Now havoc breaks lose as we have a dangling reference. The problem is not showing on all systems as they OS may not immediately reuse the destroyed objects' memory. For the possible race - that actually was not really a race - my update-comment was misleading. It was the fact that I realized that an onAny/... assignment could immediately trigger a callback invocation. Our defer certainly delays the callback but I felt that it would look much cleaner if I made sure that the initializing of authenticating happened before assigning the callback. Let me correct myself here; onAny gets called on a temporary, generates a reference and that is then referenced by `future`. - Till --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/#review78123 --- On March 25, 2015, 11:35 p.m., Till Toenshoff wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27760/ --- (Updated March 25, 2015, 11:35 p.m.) Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2050 https://issues.apache.org/jira/browse/MESOS-2050 Repository: mesos Description --- The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos. Diffs - include/mesos/authentication/authenticator.hpp f66217a src/authentication/cram_md5/authenticator.hpp c6f465f src/authentication/cram_md5/authenticator.cpp PRE-CREATION src/authentication/cram_md5/auxprop.hpp b894386 src/master/master.hpp 3c957ab src/master/master.cpp dccd7c6 src/tests/cram_md5_authentication_tests.cpp 92a89c5 Diff: https://reviews.apache.org/r/27760/diff/ Testing --- make check Thanks, Till Toenshoff
