Re: Review Request 44628: Fixed a comment and ordering in mesos.proto.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44628/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 Diff: https://reviews.apache.org/r/44628/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44634: Updated the log message in the HTTP API executor library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44634/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 Diff: https://reviews.apache.org/r/44634/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44635: Corrected the log message and variable name in executor library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44635/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e Diff: https://reviews.apache.org/r/44635/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Review Request 44709: Allowed unknown flags in command and docker executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44709/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs - src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e Diff: https://reviews.apache.org/r/44709/diff/ Testing --- `make check` on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Re: Review Request 44661: Deprecated the `docker_stop_timeout` flag.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44661/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Bugs: MESOS-4910 https://issues.apache.org/jira/browse/MESOS-4910 Repository: mesos Description --- Instead, a combination of `executor_shutdown_grace_period` agent flag and task kill policies should be used. Diffs (updated) - docs/configuration.md f6e84023b90e560594429826ed7163310d62b265 src/docker/executor.hpp abbc419533ab40312e917931a2fc2ce78b38da41 src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe Diff: https://reviews.apache.org/r/44661/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44657: Used `KillPolicy` in command executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44657/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos, Ben Mahler and Gilbert Song. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs (updated) - src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 Diff: https://reviews.apache.org/r/44657/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Review Request 44707: Added validation for task's kill policy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44707/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs - src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca Diff: https://reviews.apache.org/r/44707/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44653: Fixed formatting in command executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44653/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e Diff: https://reviews.apache.org/r/44653/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44660: Used `KillPolicy` and shutdown grace period in docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44660/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs (updated) - src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 Diff: https://reviews.apache.org/r/44660/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44659: Updated the comment about docker executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44659/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- See summary. Diffs (updated) - src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 Diff: https://reviews.apache.org/r/44659/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44662: Added kill policies and shutdown grace period to the CHANGELOG.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44662/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - CHANGELOG 1fbf3029d35207041b5204ed5754a412d5870b3c Diff: https://reviews.apache.org/r/44662/diff/ Testing (updated) --- `make check` on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Re: Review Request 44658: Removed unused signal escalation constant.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44658/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1571 https://issues.apache.org/jira/browse/MESOS-1571 Repository: mesos Description --- See summary. Diffs (updated) - src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 Diff: https://reviews.apache.org/r/44658/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44655: Made `shutdown_grace_period` configurable in `ExecutorInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44655/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Gilbert Song. Bugs: MESOS-1571 https://issues.apache.org/jira/browse/MESOS-1571 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e src/executor/executor.cpp c3e95ea7e4edf78f2a65ddc15e213aba66e69db2 src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 Diff: https://reviews.apache.org/r/44655/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44651: Fixed formatting in executor library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44651/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e Diff: https://reviews.apache.org/r/44651/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44656: Introduced `KillPolicy` protobuf.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44656/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 Diff: https://reviews.apache.org/r/44656/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Review Request 44708: Fixed signed / unsigned comparison in docker.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44708/ --- Review request for mesos, Ben Mahler and Timothy Chen. Repository: mesos Description --- See summary. Diffs - src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 Diff: https://reviews.apache.org/r/44708/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 43763: Passed `Duration` as const reference in the executor library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43763/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e Diff: https://reviews.apache.org/r/43763/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44630: Renamed `EXECUTOR_SHUTDOWN_GRACE_PERIOD` constant.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44630/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-1571 https://issues.apache.org/jira/browse/MESOS-1571 Repository: mesos Description --- See summary. Diffs (updated) - src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e src/slave/constants.hpp bcbb1401aa8f9f04c4f9256bb4f560e18d8994e0 src/slave/constants.cpp 0f0d8e4b079d136d250f83cfc768de8c98b8bee2 src/slave/flags.cpp eb470154f30634b3db439be1c122ff93d3147afe Diff: https://reviews.apache.org/r/44630/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44631: Cleaned up the comment around executor shutdown event.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44631/ --- (Updated March 11, 2016, 4 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs (updated) - include/mesos/executor/executor.proto a67dbad458e24b22875cf15b73b64c14a084b1ea include/mesos/v1/executor/executor.proto c4ad0b4ac6a4e8def013b276877a9c0a5264d4e9 Diff: https://reviews.apache.org/r/44631/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44657: Used `KillPolicy` and shutdown grace period in command executor.
> On March 15, 2016, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, lines 121-125 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line121> > > > > Ditto from previous review comments, could you adjust the comment and > > logic to reflect that it's not a default anymore? Per offline discussion, we decided to drop this code altogether. The env var has the actual value. If the env var is not set, the agent is pre 0.28 and it doesn't support custom shutdown grace periods anyway, so even if ExecutorInfo specifies the custom grace period, we can ignore it. > On March 15, 2016, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, lines 474-475 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line474> > > > > Could you be more explicit about what this guarantee is? i.e. > > mentioning that the agent, when determining the executor's shtudown grace > > period, will choose it based on the kill policy. > > > > Also, it seems surprising that killTask calls the version of shutdown > > here that doesn't take the time, why are we looking at the shutdown grace > > period if only a killTask was issued? I see why we pick the smaller if a > > shutdown was issued, but if only a killTask was issued, it seems we can > > ignore the shutdown period? I think we can go even further and simplify this code. Let me try to explain why. Command executor has a single task, hence `shutdown` and `killTask` calls are logically similar, hence exectuor shutdown grace period and kill task grace period should depend on each other. `ExecutorInfo.shutdown_grace_period` cannot be explicitly set by the user / framework for command executor. If the user omits kill policy, `ExecutorInfo.shutdown_grace_period` takes the agent default and is the only grace period available in the executor code. If the user sets kill policy, `ExecutorInfo.shutdown_grace_period` is calculated based on this value and though both grace periods are available in the executor code, we can use any. All these is not relevant for pre 0.28 agents, any value is fine this case. I would suggest to remove `min()` and *always* use `killPolicy` if set and fallback to shutdown grace period otherwise. Does it make sense? > On March 15, 2016, 10:25 p.m., Ben Mahler wrote: > > src/launcher/executor.cpp, line 499 > > <https://reviews.apache.org/r/44657/diff/4/?file=1299823#file1299823line499> > > > > I'd expect that the buffer is in addition to dealing with the reap > > interval, since the reap interval is not really a buffer but a time that we > > are expecting to wait. Could you add maybe an additional 1s buffer on top > > of the reap interval? > > > > Note that we could improve the reaper to block auxiliary threads when > > the process is a child, rather than polling for children. I had a TODO for > > this: > > > > ``` > > // TODO(bmahler): This can be optimized to use a thread per pid, where > > // each thread makes a blocking call to waitpid. This eliminates the > > // unfortunate poll delay. > > ``` I think refactoring the reaper is a great idea but maybe not now. I will add an extra time buffer and leave a todo to remove it once the reaper is updated. - Alexander ------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44657/#review123765 --- On March 15, 2016, 4:04 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44657/ > --- > > (Updated March 15, 2016, 4:04 p.m.) > > > Review request for mesos, Ben Mahler and Gilbert Song. > > > Bugs: MESOS-4909 > https://issues.apache.org/jira/browse/MESOS-4909 > > > Repository: mesos > > > Description > --- > > The command executor determines how much time it allots the > underlying task to clean up (effectively how long to wait for > the task to comply to SIGTERM before sending SIGKILL) based > on both optional task's `KillPolicy` and optional > `shutdown_grace_period` field in `ExecutorInfo`. > > > Diffs > - > > src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e > src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 > > Diff: https://reviews.apache.org/r/44657/diff/ > > > Testing > --- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 44651: Cleaned up formatting in executor library.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44651/ --- (Updated March 17, 2016, 11:27 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- The general pattern for formatting the code is to reduce jaggedness and increase readability. Diffs (updated) - src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 Diff: https://reviews.apache.org/r/44651/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 45040: Added a test for task's kill policy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45040/ --- (Updated March 19, 2016, 10:40 a.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs - src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 Diff: https://reviews.apache.org/r/45040/diff/ Testing (updated) --- On Mac OS 10.10.4: `make check` `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` Thanks, Alexander Rukletsov
Re: Review Request 44652: Omitted names of unused parameters in command executor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44652/ --- (Updated March 19, 2016, 10:31 a.m.) Review request for mesos, Ben Mahler and Joerg Schad. Repository: mesos Description --- See summary. Diffs - src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e Diff: https://reviews.apache.org/r/44652/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45039/ --- (Updated April 5, 2016, 1:27 p.m.) Review request for mesos, Ben Mahler and Neil Conway. Repository: mesos Description --- If the task does not pass validation, its resources are considered declined. Diffs (updated) - docs/app-framework-development-guide.md 1d8bebde67f69fd414509b8861571137d3569b46 include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 src/java/src/org/apache/mesos/SchedulerDriver.java bf866f5ebece2505eaa27bf39a1382cd1a2a069a src/python/interface/src/mesos/interface/__init__.py 232890daa6d222ae1c86906bbc484c8e635c4eb7 Diff: https://reviews.apache.org/r/45039/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 45804: Added a path to the upgrade test script.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45804/#review127298 --- Ship it! Let's add a JIRA ticket to description, so that "the next" guy will save some time. Also, I dream of cross-platform helper functions that will allow us to define things like helper directory once, i.e. being able to call `getTestScriptPath()` from this python helper... - Alexander Rukletsov On April 6, 2016, 5:25 a.m., Greg Mann wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45804/ > --- > > (Updated April 6, 2016, 5:25 a.m.) > > > Review request for mesos and Vinod Kone. > > > Repository: mesos > > > Description > --- > > In Mesos 0.28.0, the `MESOS_BUILD_DIR` environment variable in the test > framework was changed to `MESOS_HELPER_DIR`, and the location of the path was > altered. This patch updates the upgrade script to accommodate both > environment variables. > > > Diffs > - > > support/test-upgrade.py 99c8e7656dc2a2f54a309d4a369e73ca2ab2f4dd > > Diff: https://reviews.apache.org/r/45804/diff/ > > > Testing > --- > > Used `support/test-upgrade.py --prev="/path/to/0.27.2" > --next="/path/to/0.28.1-rc2"` to test. > > > Thanks, > > Greg Mann > >
Re: Review Request 46135: Fix the bug in MasterAllocatorTest/1.RebalancedForUpdatedWeights test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46135/#review128640 --- src/tests/master_allocator_tests.cpp (line 1604) <https://reviews.apache.org/r/46135/#comment192122> If you're not doing any checks with resources, you can use `Future` instead. src/tests/master_allocator_tests.cpp (line 1637) <https://reviews.apache.org/r/46135/#comment192121> You don't need this `settle()`. - Alexander Rukletsov On April 13, 2016, 3:21 a.m., Yongqiao Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46135/ > --- > > (Updated April 13, 2016, 3:21 a.m.) > > > Review request for mesos, Adam B and Alexander Rukletsov. > > > Bugs: MESOS-5146 > https://issues.apache.org/jira/browse/MESOS-5146 > > > Repository: mesos > > > Description > --- > > Pause the clock and make sure all resources from rescinded > offers are recovered before doing a batch allocation. > > > Diffs > - > > src/tests/master_allocator_tests.cpp > 404ff098baf89bf2a1c6e32424d591a6ea1a093c > > Diff: https://reviews.apache.org/r/46135/diff/ > > > Testing > --- > > Make && Make check. > > ``` > Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests > --gtest_filter=MasterAllocatorTest/0.RebalancedForUpdatedWeights > [==] Running 1 test from 1 test case. > [--] Global test environment set-up. > [--] 1 test from MasterAllocatorTest/0, where TypeParam = > mesos::internal::master::allocator::MesosAllocator<mesos::internal::master::allocator::HierarchicalAllocatorProcess<mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter> > > [ RUN ] MasterAllocatorTest/0.RebalancedForUpdatedWeights > [ OK ] MasterAllocatorTest/0.RebalancedForUpdatedWeights (385 ms) > [--] 1 test from MasterAllocatorTest/0 (386 ms total) > > [--] Global test environment tear-down > [==] 1 test from 1 test case ran. (396 ms total) > [ PASSED ] 1 test. > > ``` > > > Thanks, > > Yongqiao Wang > >
Re: Review Request 42342: Added a new test cases for revive offer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42342/#review128627 --- src/tests/hierarchical_allocator_tests.cpp (line 3040) <https://reviews.apache.org/r/42342/#comment192106> s/recovered/declined framework declines resources, allocator recovers src/tests/hierarchical_allocator_tests.cpp (line 3046) <https://reviews.apache.org/r/42342/#comment192108> All other tests call it "batch allocation" now. Consistency in naming is important to avoid confusion! src/tests/hierarchical_allocator_tests.cpp (line 3077) <https://reviews.apache.org/r/42342/#comment192107> Let's stay consistent and call it "a batch allocation" - Alexander Rukletsov On April 10, 2016, 1:18 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42342/ > --- > > (Updated April 10, 2016, 1:18 p.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, and Vinod Kone. > > > Bugs: MESOS-4387 > https://issues.apache.org/jira/browse/MESOS-4387 > > > Repository: mesos > > > Description > --- > > Added a new test cases for revive offer. > > > Diffs > - > > src/tests/hierarchical_allocator_tests.cpp > a5dd57a4e0c244fb099433eb7b5777982698ebfd > > Diff: https://reviews.apache.org/r/42342/diff/ > > > Testing > --- > > make > make check > GLOG_v=2 ./bin/mesos-tests.sh > --gtest_filter="HierarchicalAllocatorTest.ReviveOffers" --verbose > > > Thanks, > > Guangya Liu > >
Re: Review Request 44334: Replaced empty hashmaps with {} in allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44334/ --- (Updated April 13, 2016, 9:38 a.m.) Review request for mesos, Benjamin Bannier, Bernd Mathiske, and Ben Mahler. Repository: mesos Description --- See summary. Diffs - src/tests/hierarchical_allocator_tests.cpp 5e60cb6a7da973da7741248f3d2bf4c25330a6ea Diff: https://reviews.apache.org/r/44334/diff/ Testing --- On Mac OS X: `make check` `GTEST_FILTER="HierarchicalAllocatorTest*" ./bin/mesos-tests.sh --gtest_break_on_failure --gtest_repeat=100` Thanks, Alexander Rukletsov
Re: Review Request 44334: Replaced empty hashmaps with {} in allocator tests.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/44334/
---
(Updated April 13, 2016, 9:34 a.m.)
Review request for mesos, Benjamin Bannier, Bernd Mathiske, and Ben Mahler.
Summary (updated)
-
Replaced empty hashmaps with {} in allocator tests.
Repository: mesos
Description
---
See summary.
Diffs (updated)
-
src/tests/hierarchical_allocator_tests.cpp
5e60cb6a7da973da7741248f3d2bf4c25330a6ea
Diff: https://reviews.apache.org/r/44334/diff/
Testing (updated)
---
On Mac OS X:
`make check`
`GTEST_FILTER="HierarchicalAllocatorTest*" ./bin/mesos-tests.sh
--gtest_break_on_failure --gtest_repeat=100`
Thanks,
Alexander Rukletsov
Re: Review Request 44335: Moved variable declarations closer to where they are used.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44335/ --- (Updated April 13, 2016, 9:38 a.m.) Review request for mesos, Bernd Mathiske and Ben Mahler. Repository: mesos Description --- See summary. Diffs - src/tests/hierarchical_allocator_tests.cpp 3e4ad31925e1b815a74d67fa3962d23fa5bc89d1 Diff: https://reviews.apache.org/r/44335/diff/ Testing --- Tested as a chain in https://reviews.apache.org/r/44336/ Thanks, Alexander Rukletsov
Re: Review Request 44335: Moved variable declarations closer to where they are used.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44335/ --- (Updated April 13, 2016, 10:07 a.m.) Review request for mesos, Bernd Mathiske and Ben Mahler. Changes --- Rebased. Repository: mesos Description (updated) --- We usually declare related variables closer to each other to increase code readability. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 03064da4a5feda0d64db6175c7d5a8e3122bb67a Diff: https://reviews.apache.org/r/44335/diff/ Testing --- Tested as a chain in https://reviews.apache.org/r/44336/ Thanks, Alexander Rukletsov
Re: Review Request 44336: Removed numeric suffixes where appropriate in allocator tests.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44336/ --- (Updated April 13, 2016, 10:07 a.m.) Review request for mesos, Bernd Mathiske and Ben Mahler. Changes --- Rebased. Repository: mesos Description --- We should not enumerate variables if the corresponding instance is the only one of such type in the test. Diffs (updated) - src/tests/hierarchical_allocator_tests.cpp 03064da4a5feda0d64db6175c7d5a8e3122bb67a Diff: https://reviews.apache.org/r/44336/diff/ Testing --- On Mac OS 10.10.4: `make check` `GTEST_FILTER="HierarchicalAllocatorTest.*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure --gtest_shuffle` Thanks, Alexander Rukletsov
Re: Review Request 44335: Moved variable declarations closer to where they are used.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44335/ --- (Updated April 13, 2016, 10:08 a.m.) Review request for mesos, Bernd Mathiske and Ben Mahler. Repository: mesos Description --- We usually declare related variables closer to each other to increase code readability. Diffs - src/tests/hierarchical_allocator_tests.cpp 03064da4a5feda0d64db6175c7d5a8e3122bb67a Diff: https://reviews.apache.org/r/44335/diff/ Testing --- Tested as a chain in https://reviews.apache.org/r/44336/ Thanks, Alexander Rukletsov
Re: Review Request 46135: Fix the bug in MasterAllocatorTest/1.RebalancedForUpdatedWeights test.
> On April 13, 2016, 9:27 a.m., Alexander Rukletsov wrote: > > src/tests/master_allocator_tests.cpp, line 1604 > > <https://reviews.apache.org/r/46135/diff/1/?file=1342280#file1342280line1604> > > > > If you're not doing any checks with resources, you can use > > `Future` instead. > > Yongqiao Wang wrote: > They are different variable type, 'const mesos::Resources' can not > convert to 'const Nothing'. You use `Future` together with `FutureSatisfy`. The difference between `FutureArg` and `FutureSatisfy` is that the latter sets the future when the method is invoked, while the former _additionally_ saves the actual value of some argument. If you don't use this value later on, you don't have to use `FutureArg`. > On April 13, 2016, 9:27 a.m., Alexander Rukletsov wrote: > > src/tests/master_allocator_tests.cpp, line 1637 > > <https://reviews.apache.org/r/46135/diff/1/?file=1342280#file1342280line1637> > > > > You don't need this `settle()`. > > Yongqiao Wang wrote: > According to my understanding, call Clock::settle() after > Clock::advance(), then it can ensure allocate() is executed before do the > following check. So I think it is needed. `AWAIT_READY` does that for you if the clock is paused. Quoting BenM: "Think about it this way, if you put a `settle()` before the `AWAIT_READY`, then the implication is that you don't need `AWAIT_READY` at all, you could just do `ASSERT_TRUE(f.isReady())` because you've done the necessary settling explicitly. This is why it seems strange to have a `settle` immediately preceed an `AWAIT_READY`, its the job of `AWAIT_READY` to wait until the future is ready (which means doing the right thing when the clock is paused, otherwise we would have to write a lot of `settle` calls)." - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46135/#review128640 --- On April 13, 2016, 3:21 a.m., Yongqiao Wang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46135/ > ------- > > (Updated April 13, 2016, 3:21 a.m.) > > > Review request for mesos, Adam B and Alexander Rukletsov. > > > Bugs: MESOS-5146 > https://issues.apache.org/jira/browse/MESOS-5146 > > > Repository: mesos > > > Description > --- > > Pause the clock and make sure all resources from rescinded > offers are recovered before doing a batch allocation. > > > Diffs > - > > src/tests/master_allocator_tests.cpp > 404ff098baf89bf2a1c6e32424d591a6ea1a093c > > Diff: https://reviews.apache.org/r/46135/diff/ > > > Testing > --- > > Make && Make check. > > ``` > Yongs-MacBook-Pro:build yqwyq$ ./src/mesos-tests > --gtest_filter=MasterAllocatorTest/0.RebalancedForUpdatedWeights > [==] Running 1 test from 1 test case. > [--] Global test environment set-up. > [--] 1 test from MasterAllocatorTest/0, where TypeParam = > mesos::internal::master::allocator::MesosAllocator<mesos::internal::master::allocator::HierarchicalAllocatorProcess<mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter> > > [ RUN ] MasterAllocatorTest/0.RebalancedForUpdatedWeights > [ OK ] MasterAllocatorTest/0.RebalancedForUpdatedWeights (385 ms) > [--] 1 test from MasterAllocatorTest/0 (386 ms total) > > [--] Global test environment tear-down > [==] 1 test from 1 test case ran. (396 ms total) > [ PASSED ] 1 test. > > ``` > > > Thanks, > > Yongqiao Wang > >
Re: Review Request 44441: Treated command as executable value and arguments in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1/#review128646 --- src/cli/execute.cpp <https://reviews.apache.org/r/1/#comment192131> Why have you removed the TODO without adding a word about how `command` works wiht containers? src/cli/execute.cpp (line 339) <https://reviews.apache.org/r/1/#comment192132> I'm confused. I thought we've agreed during the offline discussion that we need to parse command properly and that `tokenize` is error-prone in this case. - Alexander Rukletsov On April 13, 2016, 2:47 a.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1/ > --- > > (Updated April 13, 2016, 2:47 a.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, haosdent huang, > and Jie Yu. > > > Bugs: MESOS-4882 > https://issues.apache.org/jira/browse/MESOS-4882 > > > Repository: mesos > > > Description > --- > > Treated command as executable value and arguments in mesos-execute. > > > Diffs > - > > src/cli/execute.cpp 306779f5227248a9f1d67b6c50854ec47dd59cd5 > > Diff: https://reviews.apache.org/r/1/diff/ > > > Testing > --- > > 1) with docker entry point > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell > I0403 21:35:42.949331 12369 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4- > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > > 2) With command 1 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd" > I0403 21:36:55.703246 12483 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0001 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 3) With command 2 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd,/etc/passwd" > I0403 21:37:22.901828 12548 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0002 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 4) With command 3 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="cat,/etc/passwd,/etc/passwd" > I0403 21:39:35.986281 12651 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0003 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 5) > root@mesos002:~/src/mesos/m2/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="echo, hello,world" > I0410 21:09:57.914875 10649 scheduler.cpp:175] Version: 0.29.0 > Subscribed with ID '11a3bd6c-be66-46d6-a5a5-8e9b635940fe-0007 > task test_mesos submitted to agent 7419ec23-763d-441f-b5d4-6d9222a1cfc9-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > From sandbox stdout: > [echo, echo, hello, world] > hello world > > 6) root@mesos002:~/src/mesos/m2/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="echo, hello world" > I0410 21:09:53.783275 10581 scheduler.cpp:175] Version: 0.29.0 > Subscribed with ID '11a3bd6c-be66-46d6-a5a5-8e9b635940fe-0006 > task test_mesos submitted to agent 7419ec23-76
Re: Review Request 46033: Removed request body from some error responses in quota handler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46033/ --- (Updated April 11, 2016, 3:16 p.m.) Review request for mesos and Joerg Schad. Repository: mesos Description --- When an error is not related to the whole request (e.g., parsing, converting to protobuf), but to its part (e.g., unknown role), it seems unnecessary to include the complete request body into the error response. Diffs (updated) - src/master/quota_handler.cpp 88247d6525b2aaeb58eea77376fd4ed8e0c653c3 Diff: https://reviews.apache.org/r/46033/diff/ Testing --- make check Thanks, Alexander Rukletsov
Re: Review Request 45984: Fixed the commit message hook to wrap the variables in quotes.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45984/#review128134 --- Ship it! Could you please briefly explain the problem with the globbing in the description? Btw, I was unaware of that patch and filed https://reviews.apache.org/r/46034/ . I'll discard it in favour of your patch. - Alexander Rukletsov On April 10, 2016, 6:41 a.m., Michael Park wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45984/ > --- > > (Updated April 10, 2016, 6:41 a.m.) > > > Review request for mesos, Joerg Schad, Kevin Klues, and Vinod Kone. > > > Bugs: MESOS-5162 > https://issues.apache.org/jira/browse/MESOS-5162 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058 > > Diff: https://reviews.apache.org/r/45984/diff/ > > > Testing > --- > > > Thanks, > > Michael Park > >
Review Request 46034: Avoided globbing in commit message hook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46034/ --- Review request for mesos, Artem Harutyunyan, Joerg Schad, and Vinod Kone. Repository: mesos Description --- Some symbols, e.g., '*' can be expanded by the shell altering the original line and hence leading to false positives. Diffs - support/hooks/commit-msg d3f10c886a654aa8d8364faddf2e50dc7fe82058 Diff: https://reviews.apache.org/r/46034/diff/ Testing --- Manually tested on commits with description containing '*'. Committing with '*' is failing without this patch with the reported line length being 400+ charachters. Thanks, Alexander Rukletsov
Review Request 46033: Removed request body from some error responses in quota handler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46033/ --- Review request for mesos and Joerg Schad. Repository: mesos Description --- When an error is not related to the whole request (e.g., parsing, converting to protobuf), but to its part (e.g., unknown role), it seems unnecessary to include the complete request body into the error response. Diffs - src/master/quota_handler.cpp 88247d6525b2aaeb58eea77376fd4ed8e0c653c3 Diff: https://reviews.apache.org/r/46033/diff/ Testing --- make check Thanks, Alexander Rukletsov
Re: Review Request 45863: Updated error messages in weights handler.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote: > > src/master/weights_handler.cpp, line 90 > > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90> > > > > Not yours: Above the request.body is included in the output. Feels > > inconsistent here... > > Alexander Rukletsov wrote: > Above are errors for the whole request, here they are per role basis. > > Joerg Schad wrote: > Ok, (unrelated to this) would you then say we should to remove in for > quota_handler as well? > ```return BadRequest( > "Failed to validate set quota request JSON '" + request.body + > "': Unknown role '" + quotaInfo.role() + "'"); > }``` > > Alexander Rukletsov wrote: > Why should we? Filed https://reviews.apache.org/r/46033/ - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/#review127580 --- On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45863/ > --- > > (Updated April 7, 2016, 10:50 a.m.) > > > Review request for mesos, Yongqiao Wang and Joerg Schad. > > > Repository: mesos > > > Description > --- > > While writing log or error messages we adhere the following style: > * No period at the end (there are exceptions though, e.g., one is > when constructing request responses. > * When splitting a string over multiple lines, put a space at the > beginning of the following line in contrast to the end of the > previous line. > > > Diffs > - > > src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e > > Diff: https://reviews.apache.org/r/45863/diff/ > > > Testing > --- > > On Mac OS 10.10.4: > `make check` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 45863: Updated error messages in weights handler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/ --- (Updated April 11, 2016, 2:48 p.m.) Review request for mesos, Yongqiao Wang and Joerg Schad. Repository: mesos Description --- While writing log or error messages we adhere the following style: * No period at the end (there are exceptions though, e.g., one is when constructing request responses. * When splitting a string over multiple lines, put a space at the beginning of the following line in contrast to the end of the previous line. Diffs (updated) - src/master/weights_handler.cpp 10a1fbc5656f98bc893cf63f2653565ae7ebd125 Diff: https://reviews.apache.org/r/45863/diff/ Testing --- On Mac OS 10.10.4: `make check` Thanks, Alexander Rukletsov
Re: Review Request 46102: Fixed logic error in execute.cpp.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/46102/#review128647 --- Ship it! Ship It! - Alexander Rukletsov On April 12, 2016, 11:34 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/46102/ > --- > > (Updated April 12, 2016, 11:34 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-5199 > https://issues.apache.org/jira/browse/MESOS-5199 > > > Repository: mesos > > > Description > --- > > Fixed logic error in execute.cpp. > > > Diffs > - > > src/cli/execute.cpp 306779f5227248a9f1d67b6c50854ec47dd59cd5 > > Diff: https://reviews.apache.org/r/46102/diff/ > > > Testing > --- > > root@mesos002:~/src/mesos/m2/mesos/build# src/mesos-execute > --master=192.168.56.12:5050 --name=test --docker_image=ubuntu:14.04 > --command="ls /root" > I0413 07:28:03.833521 2295 scheduler.cpp:175] Version: 0.29.0 > Subscribed with ID '3a1af11e-cf66-4ce2-826d-48b332977999-0001' > Submitted task 'test' to agent '3a1af11e-cf66-4ce2-826d-48b332977999-S0' > Received status update TASK_RUNNING for task 'test' > source: SOURCE_EXECUTOR > Received status update TASK_FINISHED for task 'test' > message: 'Command exited with status 0' > source: SOURCE_EXECUTOR > > > Thanks, > > Guangya Liu > >
Review Request 45927: Introduced kill task delay in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/
---
Review request for mesos, Anand Mazumdar and Joseph Wu.
Bugs: MESOS-5124
https://issues.apache.org/jira/browse/MESOS-5124
Repository: mesos
Description
---
Adds a flag that specifies a delay after which the task should be
killed. If set, a kill task request is scheduled to be sent after
TASK_RUNNING status update has been received. Also adds support for
TASK_KILLING capability.
Diffs
-
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45927/diff/
Testing
---
On Mac OS 10.10.4:
`make check`
Additionally manually tested `mesos-execute` with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
`./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": 2}'`
`./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": 2}'`
Thanks,
Alexander Rukletsov
Review Request 45925: Extended logging for task status updates in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45925/ --- Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs - src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 Diff: https://reviews.apache.org/r/45925/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Review Request 45926: Cleaned up flag descriptions in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45926/ --- Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs - src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 Diff: https://reviews.apache.org/r/45926/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Re: Review Request 43524: Speeded up RecoverTest.AutoInitialization by advacing the clock.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43524/#review127775 --- Fix it, then Ship it! Could you please update the description? As a start, you can use the comment you left on JIRA about hardcoded values in the replicated log. Also, please tun this test in repetition. src/tests/log_tests.cpp (lines 1877 - 1878) <https://reviews.apache.org/r/43524/#comment191166> Does it make sense to move this line up right after advancing the clock? - Alexander Rukletsov On Feb. 12, 2016, 2:47 p.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43524/ > --- > > (Updated Feb. 12, 2016, 2:47 p.m.) > > > Review request for mesos, Ben Mahler and haosdent huang. > > > Bugs: MESOS-4160 > https://issues.apache.org/jira/browse/MESOS-4160 > > > Repository: mesos > > > Description > --- > > Speeded up RecoverTest.AutoInitialization by advacing the clock. > > > Diffs > - > > src/tests/log_tests.cpp 923d71f48e743a77f2a3ba9e982aef4417c6c7fe > > Diff: https://reviews.apache.org/r/43524/diff/ > > > Testing > --- > > `$ sudo make check -j2 GTEST_FILTER='RecoverTest.AutoInitialization'` > > ```sh > [--] 1 test from RecoverTest > [ RUN ] RecoverTest.AutoInitialization > [ OK ] RecoverTest.AutoInitialization (630 ms) > [--] 1 test from RecoverTest (631 ms total) > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 45863: Updated error messages in weights handler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/#review127755 --- @ReviewBot retry - Alexander Rukletsov On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45863/ > --- > > (Updated April 7, 2016, 10:50 a.m.) > > > Review request for mesos, Yongqiao Wang and Joerg Schad. > > > Repository: mesos > > > Description > --- > > While writing log or error messages we adhere the following style: > * No period at the end (there are exceptions though, e.g., one is > when constructing request responses. > * When splitting a string over multiple lines, put a space at the > beginning of the following line in contrast to the end of the > previous line. > > > Diffs > - > > src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e > > Diff: https://reviews.apache.org/r/45863/diff/ > > > Testing > --- > > On Mac OS 10.10.4: > `make check` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 43329: Speeded up MasterAllocatorTest.SlaveLost test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43329/#review127783 --- Fix it, then Ship it! Could you please run tests in repetition? Thanks! src/tests/master_allocator_tests.cpp (line 652) <https://reviews.apache.org/r/43329/#comment191171> I think it's fine to do this, but let's explain why we adjust the flag. - Alexander Rukletsov On March 30, 2016, 1:22 p.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43329/ > --- > > (Updated March 30, 2016, 1:22 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-3775 > https://issues.apache.org/jira/browse/MESOS-3775 > > > Repository: mesos > > > Description > --- > > Speeded up MasterAllocatorTest.SlaveLost test. > > > Diffs > - > > src/tests/master_allocator_tests.cpp > cba7c36471f93b678d94e1da0251a28a893696b1 > > Diff: https://reviews.apache.org/r/43329/diff/ > > > Testing > --- > > ``` > [--] 1 test from MasterAllocatorTest/0, where TypeParam = > mesos::internal::master::allocator::MesosAllocator torProcess<mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter> > > [ RUN ] MasterAllocatorTest/0.SlaveLost > [ OK ] MasterAllocatorTest/0.SlaveLost (369 ms) > [--] 1 test from MasterAllocatorTest/0 (369 ms total) > > [--] 1 test from MasterAllocatorTest/1, where TypeParam = > mesos::internal::tests::Module<mesos::master::allocator::Allocator, > (mesos::internal::tests::ModuleID)6> > [ RUN ] MasterAllocatorTest/1.SlaveLost > [ OK ] MasterAllocatorTest/1.SlaveLost (120 ms) > [--] 1 test from MasterAllocatorTest/1 (120 ms total) > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 43515: Speed up MasterTest.MasterInfoOnReElection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43515/#review127788 --- Could you please run the test in repetition? src/tests/master_tests.cpp (line 1036) <https://reviews.apache.org/r/43515/#comment191176> Why do you settle? src/tests/master_tests.cpp (line 1037) <https://reviews.apache.org/r/43515/#comment191174> Let's extract this value from master flags: it's more reliable if someone decides to change interval through flags in this test. src/tests/master_tests.cpp (line 1042) <https://reviews.apache.org/r/43515/#comment191175> Move it up after `advance()`? - Alexander Rukletsov On Feb. 12, 2016, 6:36 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43515/ > --- > > (Updated Feb. 12, 2016, 6:36 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4165 > https://issues.apache.org/jira/browse/MESOS-4165 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.MasterInfoOnReElection. > > > Diffs > - > > src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 > > Diff: https://reviews.apache.org/r/43515/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42241/#review127792 --- src/tests/hook_tests.cpp (line 391) <https://reviews.apache.org/r/42241/#comment191185> I would argue that the "right thing to do" here is to explicitly call `containerizer.destroy`. `TestContainerizer` does not track whether an underlying executor has exited or not, hence we should manually destroy the container *after* executor shuts down. What you propose to do, is to wait until the agent forcibly shuts down the executor, which is fine, but not necessary, and hence may be misleading. Does it make sense? src/tests/hook_tests.cpp (line 392) <https://reviews.apache.org/r/42241/#comment191180> we don't need `settle` here because AWAIT_READY does it for us. However, we should resume the clock since in the future we may have a check that the clock is unpaused when test finishes. - Alexander Rukletsov On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42241/ > --- > > (Updated Feb. 19, 2016, 4:36 a.m.) > > > Review request for mesos, Alexander Rukletsov and Timothy Chen. > > > Bugs: MESOS-4174 > https://issues.apache.org/jira/browse/MESOS-4174 > > > Repository: mesos > > > Description > --- > > Speed up HookTest.VerifySlaveLaunchExecutorHook. > > > Diffs > - > > src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 > > Diff: https://reviews.apache.org/r/42241/diff/ > > > Testing > --- > > Before > HookTest.VerifySlaveLaunchExecutorHook (5061 ms) > > After > HookTest.VerifySlaveLaunchExecutorHook (132 ms) > > > Thanks, > > Jian Qiu > >
Re: Review Request 43514: Speed up MasterTest.RecoverResources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43514/#review127786 --- Fix it, then Ship it! Mind adding the description and do tests in repetition? src/tests/master_tests.cpp (line 745) <https://reviews.apache.org/r/43514/#comment191172> I believe you do settle in order to make sure the container is destroyed. Mind writing a comment? Also, I think you can resume the clock right after advancing. - Alexander Rukletsov On Feb. 12, 2016, 6:36 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43514/ > --- > > (Updated Feb. 12, 2016, 6:36 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4164 > https://issues.apache.org/jira/browse/MESOS-4164 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.RecoverResources. > > > Diffs > - > > src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 > > Diff: https://reviews.apache.org/r/43514/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 43523: Speed up SlaveTest.MetricsSlaveLaunchErrors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43523/#review127784 --- Do we still need this patch? I believe Benjamin has fixed rate limiting for metrcis in tests altogether with https://reviews.apache.org/r/44073/. Correct? - Alexander Rukletsov On Feb. 12, 2016, 8:09 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43523/ > --- > > (Updated Feb. 12, 2016, 8:09 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4162 > https://issues.apache.org/jira/browse/MESOS-4162 > > > Repository: mesos > > > Description > --- > > Speed up SlaveTest.MetricsSlaveLaunchErrors. > > > Diffs > - > > src/tests/slave_tests.cpp c7f5a701eff2c2f9aa3df5722583a131bf2c072a > > Diff: https://reviews.apache.org/r/43523/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.
> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote: > > src/tests/hook_tests.cpp, line 396 > > <https://reviews.apache.org/r/42241/diff/3/?file=1200496#file1200496line396> > > > > Is there a need to explicitly invoke `Clock::resume()` here? If not, > > kill it. > > Alexander Rukletsov wrote: > Let's leave the test with resumed clock for consistency. > > Anand Mazumdar wrote: > Alex, what consistency are you referring to here? Does this file follow > the pattern of explicitly doing so? We've discussed it with BenM here recently: https://reviews.apache.org/r/44994/#comment186808 - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42241/#review119797 --- On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42241/ > --- > > (Updated Feb. 19, 2016, 4:36 a.m.) > > > Review request for mesos, Alexander Rukletsov and Timothy Chen. > > > Bugs: MESOS-4174 > https://issues.apache.org/jira/browse/MESOS-4174 > > > Repository: mesos > > > Description > --- > > Speed up HookTest.VerifySlaveLaunchExecutorHook. > > > Diffs > - > > src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 > > Diff: https://reviews.apache.org/r/42241/diff/ > > > Testing > --- > > Before > HookTest.VerifySlaveLaunchExecutorHook (5061 ms) > > After > HookTest.VerifySlaveLaunchExecutorHook (132 ms) > > > Thanks, > > Jian Qiu > >
Re: Review Request 43514: Speed up MasterTest.RecoverResources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43514/#review127787 --- src/tests/master_tests.cpp (line 68) <https://reviews.apache.org/r/43514/#comment191173> After the second thought, a better approach would be to create an instance of master flags and extract the allocator interval from there. - Alexander Rukletsov On Feb. 12, 2016, 6:36 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43514/ > --- > > (Updated Feb. 12, 2016, 6:36 a.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4164 > https://issues.apache.org/jira/browse/MESOS-4164 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.RecoverResources. > > > Diffs > - > > src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 > > Diff: https://reviews.apache.org/r/43514/diff/ > > > Testing > --- > > > Thanks, > > haosdent huang > >
Re: Review Request 43480: Use in_memory as default registry when testing.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43480/#review127790 --- Joris started a similar effort some time ago: https://reviews.apache.org/r/41665/ - Alexander Rukletsov On Feb. 11, 2016, 4:02 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43480/ > --- > > (Updated Feb. 11, 2016, 4:02 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4647 > https://issues.apache.org/jira/browse/MESOS-4647 > > > Repository: mesos > > > Description > --- > > Use in_memory as default registry when testing. > > > Diffs > - > > src/tests/master_tests.cpp 393a6f5fe3744d6ba743f362b7e309d1ee75a303 > src/tests/mesos.hpp c0997dbacf8753149caba0a8b2406afc35a4845f > src/tests/mesos.cpp e0f641c6828833de13a0a233e39ff6dc3f343d5c > > Diff: https://reviews.apache.org/r/43480/diff/ > > > Testing > --- > > sudo test in CentOS 7.1 > > > Thanks, > > haosdent huang > >
Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.
> On Feb. 19, 2016, 2:50 a.m., Anand Mazumdar wrote: > > src/tests/hook_tests.cpp, line 396 > > <https://reviews.apache.org/r/42241/diff/3/?file=1200496#file1200496line396> > > > > Is there a need to explicitly invoke `Clock::resume()` here? If not, > > kill it. Let's leave the test with resumed clock for consistency. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42241/#review119797 --- On Feb. 19, 2016, 4:36 a.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42241/ > --- > > (Updated Feb. 19, 2016, 4:36 a.m.) > > > Review request for mesos, Alexander Rukletsov and Timothy Chen. > > > Bugs: MESOS-4174 > https://issues.apache.org/jira/browse/MESOS-4174 > > > Repository: mesos > > > Description > --- > > Speed up HookTest.VerifySlaveLaunchExecutorHook. > > > Diffs > - > > src/tests/hook_tests.cpp 59a00ea722a17bbc82b14c69bda826f68cbac6e9 > > Diff: https://reviews.apache.org/r/42241/diff/ > > > Testing > --- > > Before > HookTest.VerifySlaveLaunchExecutorHook (5061 ms) > > After > HookTest.VerifySlaveLaunchExecutorHook (132 ms) > > > Thanks, > > Jian Qiu > >
Re: Review Request 43321: Speeded up SchedulerTest.Decline by advancing the clock.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43321/#review127796 --- Could you please rebase it? - Alexander Rukletsov On March 30, 2016, 1:22 p.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43321/ > --- > > (Updated March 30, 2016, 1:22 p.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-4175 > https://issues.apache.org/jira/browse/MESOS-4175 > > > Repository: mesos > > > Description > --- > > Speeded up SchedulerTest.Decline by advancing the clock. > > > Diffs > - > > src/tests/scheduler_tests.cpp 70c5b218aa231b21277580567d92f31c16a95efb > > Diff: https://reviews.apache.org/r/43321/diff/ > > > Testing > --- > > sudo make check -j2 GTEST_FILTER='ContentType/SchedulerTest.Decline/* > > ```sh > [ RUN ] ContentType/SchedulerTest.Decline/0 > [ OK ] ContentType/SchedulerTest.Decline/0 (114 ms) > [ RUN ] ContentType/SchedulerTest.Decline/1 > [ OK ] ContentType/SchedulerTest.Decline/1 (98 ms) > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 45863: Updated error messages in weights handler.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote: > > src/master/weights_handler.cpp, line 90 > > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90> > > > > Not yours: Above the request.body is included in the output. Feels > > inconsistent here... > > Alexander Rukletsov wrote: > Above are errors for the whole request, here they are per role basis. > > Joerg Schad wrote: > Ok, (unrelated to this) would you then say we should to remove in for > quota_handler as well? > ```return BadRequest( > "Failed to validate set quota request JSON '" + request.body + > "': Unknown role '" + quotaInfo.role() + "'"); > }``` Why should we? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/#review127580 --- On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45863/ > --- > > (Updated April 7, 2016, 10:50 a.m.) > > > Review request for mesos, Yongqiao Wang and Joerg Schad. > > > Repository: mesos > > > Description > --- > > While writing log or error messages we adhere the following style: > * No period at the end (there are exceptions though, e.g., one is > when constructing request responses. > * When splitting a string over multiple lines, put a space at the > beginning of the following line in contrast to the end of the > previous line. > > > Diffs > - > > src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e > > Diff: https://reviews.apache.org/r/45863/diff/ > > > Testing > --- > > On Mac OS 10.10.4: > `make check` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 45863: Updated error messages in weights handler.
> On April 7, 2016, 11:07 a.m., Joerg Schad wrote: > > src/master/weights_handler.cpp, line 90 > > <https://reviews.apache.org/r/45863/diff/1/?file=1329646#file1329646line90> > > > > Not yours: Above the request.body is included in the output. Feels > > inconsistent here... Above are errors for the whole request, here they are per role basis. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/#review127580 --- On April 7, 2016, 10:50 a.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45863/ > --- > > (Updated April 7, 2016, 10:50 a.m.) > > > Review request for mesos, Yongqiao Wang and Joerg Schad. > > > Repository: mesos > > > Description > --- > > While writing log or error messages we adhere the following style: > * No period at the end (there are exceptions though, e.g., one is > when constructing request responses. > * When splitting a string over multiple lines, put a space at the > beginning of the following line in contrast to the end of the > previous line. > > > Diffs > - > > src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e > > Diff: https://reviews.apache.org/r/45863/diff/ > > > Testing > --- > > On Mac OS 10.10.4: > `make check` > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 45927: Introduced kill task delay in mesos-execute.
> On April 9, 2016, 2:32 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 157
> > <https://reviews.apache.org/r/45927/diff/1/?file=1337077#file1337077line157>
> >
> > I can see many people ask the format of `time` in user list, it would
> > be great if we can list all supported unit here.
> >
> >
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp#L53-L68
Instead of doing it on per-flag basis, how about updating it in one place,
specifically
https://github.com/apache/mesos/blob/4dfa91fc21f80204f5125b2e2f35c489f8fb41d8/3rdparty/libprocess/3rdparty/stout/include/stout/duration.hpp#L70
? Do you want to file a JIRA? I'll be happy to shepherd.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/#review127946
-------
On April 8, 2016, 12:39 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45927/
> ---
>
> (Updated April 8, 2016, 12:39 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Joseph Wu.
>
>
> Bugs: MESOS-5124
> https://issues.apache.org/jira/browse/MESOS-5124
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Adds a flag that specifies a delay after which the task should be
> killed. If set, a kill task request is scheduled to be sent after
> TASK_RUNNING status update has been received. Also adds support for
> TASK_KILLING capability.
>
>
> Diffs
> -
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45927/diff/
>
>
> Testing
> ---
>
> On Mac OS 10.10.4:
> `make check`
>
> Additionally manually tested `mesos-execute` with both responsive and
> unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
> `./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
> --env='{"GLOG_v": 2}'`
> `./src/mesos-execute --master=127.0.0.1:5050 --name=test
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": 2}'`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.
> On April 9, 2016, 2:24 a.m., Guangya Liu wrote: > > src/cli/execute.cpp, line 100 > > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100> > > > > I think we cannot sepcify the entrypoint for container here, the > > entrypoint was runtime configuration and was get from docker iamge. Why not? That's exactly what you do in https://reviews.apache.org/r/1/ - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45926/#review127945 --- On April 8, 2016, 12:39 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45926/ > --- > > (Updated April 8, 2016, 12:39 p.m.) > > > Review request for mesos, Anand Mazumdar and Joseph Wu. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 > > Diff: https://reviews.apache.org/r/45926/diff/ > > > Testing > --- > > See the last patch in the chain: https://reviews.apache.org/r/45927/ > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.
> On April 8, 2016, 6:10 p.m., Joseph Wu wrote: > > src/cli/execute.cpp, lines 97-100 > > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line97> > > > > Might be nice to note that this is ignored if `flags.shell` is false. It's true for the _current_ code in mesos-execute, but not true for Mesos. Moreover, there is a patch in the works (https://reviews.apache.org/r/1/) which adds support for this. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45926/#review127827 --- On April 8, 2016, 12:39 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45926/ > --- > > (Updated April 8, 2016, 12:39 p.m.) > > > Review request for mesos, Anand Mazumdar and Joseph Wu. > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 > > Diff: https://reviews.apache.org/r/45926/diff/ > > > Testing > --- > > See the last patch in the chain: https://reviews.apache.org/r/45927/ > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 46044: Cleaned up c-tor signature in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46044/
---
(Updated April 12, 2016, 9:59 a.m.)
Review request for mesos, Anand Mazumdar and Joseph Wu.
Repository: mesos
Description
---
Pass complete `flags` instance rather than each flag value separately
to `CommandScheduler` in mesos-execute for brevity.
Diffs (updated)
-
src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40
Diff: https://reviews.apache.org/r/46044/diff/
Testing
---
On Mac OS 10.10.4:
make check
Additionally manually tested mesos-execute with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": "2"}' --kill_after=2secs
./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
--kill_after=2secs
Thanks,
Alexander Rukletsov
Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45926/ --- (Updated April 12, 2016, 9:58 a.m.) Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs (updated) - src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 Diff: https://reviews.apache.org/r/45926/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Re: Review Request 46044: Cleaned up c-tor signature in mesos-execute.
> On April 11, 2016, 9:53 p.m., Jojy Varghese wrote:
> > src/cli/execute.cpp, line 196
> > <https://reviews.apache.org/r/46044/diff/1/?file=1339684#file1339684line196>
> >
> > I like the idea of simplifying the ctor. I am not too excited about the
> > idea of moving everything to 'flag'. A `CommandScheduler` object should
> > have some properties like `command`, `master`, `name`. Others like 'image'
> > information should be moved to its own class/struct (say `ContainerInfo`).
> > Just my 2 cents.
>
> Joseph Wu wrote:
> I partially agree, but in this case, the `Flags` class is effectively the
> class/struct you're asking for. It just happens to have everything packed
> along with it :)
>
> Jojy Varghese wrote:
> hmm i could extend that argument to get away with any type system or
> class design and replace all types with a `flag` type. Good design would
> allow translation between a user input (flag) to a CommandScheduler object.
> Passing a user input to CommandScheduler does not seem right. I think this
> was the reason the original author wanted input validation to happen before
> the object was created. CommandScheduler should only accept a strongly typed
> input in its ctor. I exceeded my 2 cents :|
>
> Joseph Wu wrote:
> I agree about input validation. All our new code puts flag validation
> directly inside the flags.
> i.e.
> https://github.com/apache/mesos/blob/master/src/master/flags.cpp#L420-L426
>
> The existing error checking inside the `main` method should be moved into
> lambdas. At which point, passing `Flags` around would be equivalent to the
> previous explicit c-tor.
We still validate flags in `main()` before we create an instance of
`CommandScheduler`. Moving validations into lambdas makes sense.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46044/#review128265
---
On April 11, 2016, 7:01 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46044/
> ---
>
> (Updated April 11, 2016, 7:01 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Pass complete `flags` instance rather than each flag value separately
> to `CommandScheduler` in mesos-execute for brevity.
>
>
> Diffs
> -
>
> src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40
>
> Diff: https://reviews.apache.org/r/46044/diff/
>
>
> Testing
> ---
>
> On Mac OS 10.10.4:
> make check
>
> Additionally manually tested mesos-execute with both responsive and
> unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
> --env='{"GLOG_v": "2"}' --kill_after=2secs
> ./src/mesos-execute --master=127.0.0.1:5050 --name=test
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
> --kill_after=2secs
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.
> On April 9, 2016, 2:24 a.m., Guangya Liu wrote:
> > src/cli/execute.cpp, line 100
> > <https://reviews.apache.org/r/45926/diff/1/?file=1337076#file1337076line100>
> >
> > I think we cannot sepcify the entrypoint for container here, the
> > entrypoint was runtime configuration and was get from docker iamge.
>
> Alexander Rukletsov wrote:
> Why not? That's exactly what you do in https://reviews.apache.org/r/1/
>
> Guangya Liu wrote:
> In my understanding, what I did in r1 is for
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L248-L256
> , but the entrypoint is
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/docker/runtime.cpp#L240-L248
> , make sense?
No, it doesn't. You have branching in your patch `if (command.isSome()) {`
which switches between the two cases and hence covers them both.
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45926/#review127945
---
On April 11, 2016, 7 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45926/
> ---
>
> (Updated April 11, 2016, 7 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Joseph Wu.
>
>
> Repository: mesos
>
>
> Description
> ---
>
> See summary.
>
>
> Diffs
> -
>
> src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40
>
> Diff: https://reviews.apache.org/r/45926/diff/
>
>
> Testing
> ---
>
> See the last patch in the chain: https://reviews.apache.org/r/45927/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 45927: Introduced kill task delay in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/
---
(Updated April 12, 2016, 9:59 a.m.)
Review request for mesos, Anand Mazumdar and Joseph Wu.
Bugs: MESOS-5124
https://issues.apache.org/jira/browse/MESOS-5124
Repository: mesos
Description
---
Adds a flag that specifies a delay after which the task should be
killed. If set, a kill task request is scheduled to be sent after
TASK_RUNNING status update has been received. Also adds support for
TASK_KILLING capability.
Diffs (updated)
-
src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40
Diff: https://reviews.apache.org/r/45927/diff/
Testing
---
On Mac OS 10.10.4:
`make check`
Additionally manually tested `mesos-execute` with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
`./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": "2"}' --kill_after=2secs`
`./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
--kill_after=2secs`
Thanks,
Alexander Rukletsov
Re: Review Request 45925: Extended logging for task status updates in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45925/ --- (Updated April 12, 2016, 9:58 a.m.) Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs (updated) - src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 Diff: https://reviews.apache.org/r/45925/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Re: Review Request 43524: Speeded up RecoverTest.AutoInitialization by advacing the clock.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43524/#review128367 --- Fix it, then Ship it! I'll adjust the comment and commit description and then commit it for you. src/tests/log_tests.cpp (line 1870) <https://reviews.apache.org/r/43524/#comment191803> s/is is/are in Also we backtick variables and types - Alexander Rukletsov On April 9, 2016, 4:11 a.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43524/ > --- > > (Updated April 9, 2016, 4:11 a.m.) > > > Review request for mesos, Alexander Rukletsov, Ben Mahler, and haosdent huang. > > > Bugs: MESOS-4160 > https://issues.apache.org/jira/browse/MESOS-4160 > > > Repository: mesos > > > Description > --- > > Speeded up RecoverTest.AutoInitialization by advacing the clock. > > > Diffs > - > > src/tests/log_tests.cpp 8f208bac13a7276074278213119d8894766e84ea > > Diff: https://reviews.apache.org/r/43524/diff/ > > > Testing > --- > > `$ sudo make check -j2 GTEST_FILTER='RecoverTest.AutoInitialization'` > > ```sh > [--] 1 test from RecoverTest > [ RUN ] RecoverTest.AutoInitialization > [ OK ] RecoverTest.AutoInitialization (630 ms) > [--] 1 test from RecoverTest (631 ms total) > ``` > > Repeatly tested with: > > ``` > ./bin/mesos-tests.sh --gtest_filter=RecoverTest.AutoInitialization > --gtest_repeat=1000 --gtest_break_on_failure' > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 43514: Speed up MasterTest.RecoverResources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43514/#review128370 --- Ship it! I'll reword the comment and the description and commit it shortly. Note that we use past tense in the summary and present in description. - Alexander Rukletsov On April 8, 2016, 6:32 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43514/ > --- > > (Updated April 8, 2016, 6:32 p.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4164 > https://issues.apache.org/jira/browse/MESOS-4164 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.RecoverResources by advance allocation interval. > > > Diffs > - > > src/tests/master_tests.cpp 8f93fbaf2bfd66bbc144a85c0097f45c55ff3491 > > Diff: https://reviews.apache.org/r/43514/diff/ > > > Testing > --- > > Repeat test in CentOS 7.1 > ``` > sudo ./bin/mesos-tests.sh --gtest_filter="MasterTest.RecoverResources" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43521: Speed up OversubscriptionTest.UpdateAllocatorOnSchedulerFailover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43521/#review128384 --- Fix it, then Ship it! src/tests/oversubscription_tests.cpp (line 1000) <https://reviews.apache.org/r/43521/#comment191820> I understand why you need to make this change now, but I think we can kill this comment, because it's not surprising that we wait for registration first and then for offers. - Alexander Rukletsov On April 8, 2016, 6:38 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43521/ > --- > > (Updated April 8, 2016, 6:38 p.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4170 > https://issues.apache.org/jira/browse/MESOS-4170 > > > Repository: mesos > > > Description > --- > > Speed up OversubscriptionTest.UpdateAllocatorOnSchedulerFailover by advance > allocation interval. > > > Diffs > - > > src/tests/oversubscription_tests.cpp > 23671746da2ac505d75bc2bd59114697d9161d52 > > Diff: https://reviews.apache.org/r/43521/diff/ > > > Testing > --- > > Repeat test on CentOS 7.1 > > ``` > sudo ./bin/mesos-tests.sh > --gtest_filter="OversubscriptionTest.UpdateAllocatorOnSchedulerFailover" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43515: Speed up MasterTest.MasterInfoOnReElection.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43515/#review128378 --- Ship it! I'll adjust the wording and will commit shortly. - Alexander Rukletsov On April 9, 2016, 6:19 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43515/ > --- > > (Updated April 9, 2016, 6:19 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4165 > https://issues.apache.org/jira/browse/MESOS-4165 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.MasterInfoOnReElection by advance allocation > interval. > > > Diffs > - > > src/tests/master_tests.cpp 8f93fbaf2bfd66bbc144a85c0097f45c55ff3491 > > Diff: https://reviews.apache.org/r/43515/diff/ > > > Testing > --- > > Repeat test in CentOS 7.1 > ``` > sudo ./bin/mesos-tests.sh --gtest_filter="MasterTest.MasterInfoOnReElection" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43516: Speed up MasterTest.LaunchCombinedOfferTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43516/#review128382 --- Ship it! I'll adjust the wording and will commit shortly. - Alexander Rukletsov On April 9, 2016, 6:19 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43516/ > --- > > (Updated April 9, 2016, 6:19 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4166 > https://issues.apache.org/jira/browse/MESOS-4166 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.LaunchCombinedOfferTest by advance allocation > interval. > > > Diffs > - > > src/tests/master_tests.cpp 8f93fbaf2bfd66bbc144a85c0097f45c55ff3491 > > Diff: https://reviews.apache.org/r/43516/diff/ > > > Testing > --- > > Repeat test in CentOS 7.1 > ``` > sudo ./bin/mesos-tests.sh --gtest_filter="MasterTest.LaunchCombinedOfferTest" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43329: Speeded up MasterAllocatorTest.SlaveLost test.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43329/#review128368 --- Fix it, then Ship it! I'll change the wording and commit shortly. src/tests/master_allocator_tests.cpp (lines 669 - 670) <https://reviews.apache.org/r/43329/#comment191804> How about: ``` // Set the `executor_shutdown_grace_period` to a small value so that // the agent does not wait for executors to clean up for too long. ``` - Alexander Rukletsov On April 9, 2016, 3:39 a.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43329/ > --- > > (Updated April 9, 2016, 3:39 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-3775 > https://issues.apache.org/jira/browse/MESOS-3775 > > > Repository: mesos > > > Description > --- > > Speeded up MasterAllocatorTest.SlaveLost test. > > > Diffs > - > > src/tests/master_allocator_tests.cpp > 404ff098baf89bf2a1c6e32424d591a6ea1a093c > > Diff: https://reviews.apache.org/r/43329/diff/ > > > Testing > --- > > ``` > [--] 1 test from MasterAllocatorTest/0, where TypeParam = > mesos::internal::master::allocator::MesosAllocator torProcess<mesos::internal::master::allocator::DRFSorter, > mesos::internal::master::allocator::DRFSorter> > > [ RUN ] MasterAllocatorTest/0.SlaveLost > [ OK ] MasterAllocatorTest/0.SlaveLost (369 ms) > [--] 1 test from MasterAllocatorTest/0 (369 ms total) > > [--] 1 test from MasterAllocatorTest/1, where TypeParam = > mesos::internal::tests::Module<mesos::master::allocator::Allocator, > (mesos::internal::tests::ModuleID)6> > [ RUN ] MasterAllocatorTest/1.SlaveLost > [ OK ] MasterAllocatorTest/1.SlaveLost (120 ms) > [--] 1 test from MasterAllocatorTest/1 (120 ms total) > ``` > > Tested repeatedly with: > ``` > ./bin/mesos-tests.sh --gtest_filter=MasterAllocatorTest*.SlaveLost > --gtest_repeat=1000 --gtest_break_on_failure' > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 43517: Speed up MasterTest.OfferTimeout.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43517/#review128383 --- Ship it! I'll adjust the wording and will commit shortly. - Alexander Rukletsov On April 8, 2016, 3:43 p.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43517/ > --- > > (Updated April 8, 2016, 3:43 p.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4167 > https://issues.apache.org/jira/browse/MESOS-4167 > > > Repository: mesos > > > Description > --- > > Speed up MasterTest.OfferTimeout by advance allocation interval. > > > Diffs > - > > src/tests/master_tests.cpp 8f93fbaf2bfd66bbc144a85c0097f45c55ff3491 > > Diff: https://reviews.apache.org/r/43517/diff/ > > > Testing > --- > > Repeat test in CentOS 7.1 > ``` > sudo ./bin/mesos-tests.sh --gtest_filter="MasterTest.OfferTimeout" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43522: Speed up OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43522/#review128385 --- Fix it, then Ship it! I'll adjust the wording and will commit shortly. src/tests/oversubscription_tests.cpp (line 1037) <https://reviews.apache.org/r/43522/#comment191822> We should either kill this comment or pull it up one line. - Alexander Rukletsov On April 9, 2016, 6:16 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43522/ > --- > > (Updated April 9, 2016, 6:16 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4171 > https://issues.apache.org/jira/browse/MESOS-4171 > > > Repository: mesos > > > Description > --- > > Speed up OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover by > advance allocation interval. > > > Diffs > - > > src/tests/oversubscription_tests.cpp > 23671746da2ac505d75bc2bd59114697d9161d52 > > Diff: https://reviews.apache.org/r/43522/diff/ > > > Testing > --- > > Repeat test on CentOS 7.1 > > ``` > sudo ./bin/mesos-tests.sh > --gtest_filter="OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 42241: Speed up HookTest.VerifySlaveLaunchExecutorHook.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/42241/#review128422 --- Ship it! Ship It! - Alexander Rukletsov On April 9, 2016, 2:45 p.m., Jian Qiu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/42241/ > --- > > (Updated April 9, 2016, 2:45 p.m.) > > > Review request for mesos, Alexander Rukletsov and Timothy Chen. > > > Bugs: MESOS-4174 > https://issues.apache.org/jira/browse/MESOS-4174 > > > Repository: mesos > > > Description > --- > > Speed up HookTest.VerifySlaveLaunchExecutorHook. > > > Diffs > - > > src/tests/hook_tests.cpp 0bbb7ac9e80234920814e34286ea0da9b648ebe3 > > Diff: https://reviews.apache.org/r/42241/diff/ > > > Testing > --- > > Before > HookTest.VerifySlaveLaunchExecutorHook (5061 ms) > > After > HookTest.VerifySlaveLaunchExecutorHook (132 ms) > > > Thanks, > > Jian Qiu > >
Re: Review Request 43522: Speed up OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43522/#review128418 --- src/tests/oversubscription_tests.cpp (line 1129) <https://reviews.apache.org/r/43522/#comment191841> I observe this test taking more than 1s on my machine. I believe we should make sure resources are recovered before we advance the clock. Adding `settle()` should do the job. I wonder why you haven't observed that at least once when running tests in repetition. - Alexander Rukletsov On April 9, 2016, 6:16 a.m., haosdent huang wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43522/ > --- > > (Updated April 9, 2016, 6:16 a.m.) > > > Review request for mesos, Alexander Rukletsov and Ben Mahler. > > > Bugs: MESOS-4171 > https://issues.apache.org/jira/browse/MESOS-4171 > > > Repository: mesos > > > Description > --- > > Speed up OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover by > advance allocation interval. > > > Diffs > - > > src/tests/oversubscription_tests.cpp > 23671746da2ac505d75bc2bd59114697d9161d52 > > Diff: https://reviews.apache.org/r/43522/diff/ > > > Testing > --- > > Repeat test on CentOS 7.1 > > ``` > sudo ./bin/mesos-tests.sh > --gtest_filter="OversubscriptionTest.RemoveCapabilitiesOnSchedulerFailover" > --gtest_repeat=200 --gtest_break_on_failure > ``` > > > Thanks, > > haosdent huang > >
Re: Review Request 43321: Speeded up SchedulerTest.Decline by advancing the clock.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/43321/#review128423 --- Fix it, then Ship it! I'll fix the last nit and commit shortly. src/tests/scheduler_tests.cpp (line 1075) <https://reviews.apache.org/r/43321/#comment191848> Any reason why not resuming right after advancing? - Alexander Rukletsov On April 9, 2016, 9:17 a.m., Shuai Lin wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/43321/ > --- > > (Updated April 9, 2016, 9:17 a.m.) > > > Review request for mesos and Alexander Rukletsov. > > > Bugs: MESOS-4175 > https://issues.apache.org/jira/browse/MESOS-4175 > > > Repository: mesos > > > Description > --- > > Speeded up SchedulerTest.Decline by advancing the clock. > > > Diffs > - > > src/tests/scheduler_tests.cpp 7956da3480c06637b6893e05f2ed92c4bb8d089f > > Diff: https://reviews.apache.org/r/43321/diff/ > > > Testing > --- > > sudo make check -j2 GTEST_FILTER='ContentType/SchedulerTest.Decline/* > > ```sh > [ RUN ] ContentType/SchedulerTest.Decline/0 > [ OK ] ContentType/SchedulerTest.Decline/0 (114 ms) > [ RUN ] ContentType/SchedulerTest.Decline/1 > [ OK ] ContentType/SchedulerTest.Decline/1 (98 ms) > ``` > > Repeatedly tested with: > > ```sh > ./bin/mesos-tests.sh --gtest_filter=ContentType/SchedulerTest.Decline/* > --gtest_repeat=1000 --gtest_break_on_failure > ``` > > > Thanks, > > Shuai Lin > >
Re: Review Request 44441: Treated command as executable value and arguments in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1/#review128027 --- src/cli/execute.cpp (line 94) <https://reviews.apache.org/r/1/#comment191422> You lost a period : ) - Alexander Rukletsov On April 3, 2016, 1:42 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1/ > --- > > (Updated April 3, 2016, 1:42 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, haosdent huang, > and Jie Yu. > > > Bugs: MESOS-4882 > https://issues.apache.org/jira/browse/MESOS-4882 > > > Repository: mesos > > > Description > --- > > Treated command as executable value and arguments in mesos-execute. > > > Diffs > - > > src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 > > Diff: https://reviews.apache.org/r/1/diff/ > > > Testing > --- > > 1) with docker entry point > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell > I0403 21:35:42.949331 12369 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4- > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > > 2) With command 1 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd" > I0403 21:36:55.703246 12483 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0001 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 3) With command 2 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd,/etc/passwd" > I0403 21:37:22.901828 12548 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0002 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 4) With command 3 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="cat,/etc/passwd,/etc/passwd" > I0403 21:39:35.986281 12651 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0003 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > > Thanks, > > Guangya Liu > >
Review Request 45863: Updated error messages in weights handler.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45863/ --- Review request for mesos, Yongqiao Wang and Joerg Schad. Repository: mesos Description --- While writing log or error messages we adhere the following style: * No period at the end (there are exceptions though, e.g., one is when constructing request responses. * When splitting a string over multiple lines, put a space at the beginning of the following line in contrast to the end of the previous line. Diffs - src/master/weights_handler.cpp e88bf2ab67ccadf35879b92f3280298a43d7cd0e Diff: https://reviews.apache.org/r/45863/diff/ Testing --- On Mac OS 10.10.4: `make check` Thanks, Alexander Rukletsov
Re: Review Request 45925: Extended logging for task status updates in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45925/ --- (Updated April 11, 2016, 6:59 p.m.) Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs (updated) - src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 Diff: https://reviews.apache.org/r/45925/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Re: Review Request 45927: Introduced kill task delay in mesos-execute.
> On April 8, 2016, 6:14 p.m., Joseph Wu wrote:
> > src/cli/execute.cpp, lines 639-651
> > <https://reviews.apache.org/r/45927/diff/1/?file=1337077#file1337077line639>
> >
> > Given how these flags are pretty much all passed into the
> > `CommandScheduler`, can you add another patch that changes the constructor
> > signature to something like:
> > ```
> > Command(const Flags& _flags)
> > : flags(flags)
> > { ... }
> > ```
See https://reviews.apache.org/r/46044/
- Alexander
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/#review127829
-------
On April 11, 2016, 4:55 p.m., Alexander Rukletsov wrote:
>
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45927/
> ---
>
> (Updated April 11, 2016, 4:55 p.m.)
>
>
> Review request for mesos, Anand Mazumdar and Joseph Wu.
>
>
> Bugs: MESOS-5124
> https://issues.apache.org/jira/browse/MESOS-5124
>
>
> Repository: mesos
>
>
> Description
> ---
>
> Adds a flag that specifies a delay after which the task should be
> killed. If set, a kill task request is scheduled to be sent after
> TASK_RUNNING status update has been received. Also adds support for
> TASK_KILLING capability.
>
>
> Diffs
> -
>
> src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
>
> Diff: https://reviews.apache.org/r/45927/diff/
>
>
> Testing
> ---
>
> On Mac OS 10.10.4:
> `make check`
>
> Additionally manually tested `mesos-execute` with both responsive and
> unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
> `./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
> --env='{"GLOG_v": "2"}' --kill_after=2secs`
> `./src/mesos-execute --master=127.0.0.1:5050 --name=test
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
> --kill_after=2secs`
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Review Request 46044: Cleaned up c-tor signature in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46044/
---
Review request for mesos, Anand Mazumdar and Joseph Wu.
Repository: mesos
Description
---
Pass complete `flags` instance rather than each flag value separately
to `CommandScheduler` in mesos-execute for brevity.
Diffs
-
src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40
Diff: https://reviews.apache.org/r/46044/diff/
Testing
---
On Mac OS 10.10.4:
make check
Additionally manually tested mesos-execute with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": "2"}' --kill_after=2secs
./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
--kill_after=2secs
Thanks,
Alexander Rukletsov
Re: Review Request 44441: Treated command as executable value and arguments in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/1/#review128216 --- src/cli/execute.cpp (line 306) <https://reviews.apache.org/r/1/#comment191605> Let's write a comment here why `command` can be `None`. - Alexander Rukletsov On April 10, 2016, 1:12 p.m., Guangya Liu wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/1/ > --- > > (Updated April 10, 2016, 1:12 p.m.) > > > Review request for mesos, Alexander Rukletsov, Gilbert Song, haosdent huang, > and Jie Yu. > > > Bugs: MESOS-4882 > https://issues.apache.org/jira/browse/MESOS-4882 > > > Repository: mesos > > > Description > --- > > Treated command as executable value and arguments in mesos-execute. > > > Diffs > - > > src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 > > Diff: https://reviews.apache.org/r/1/diff/ > > > Testing > --- > > 1) with docker entry point > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell > I0403 21:35:42.949331 12369 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4- > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > > 2) With command 1 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd" > I0403 21:36:55.703246 12483 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0001 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 3) With command 2 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="ls,/etc/passwd,/etc/passwd" > I0403 21:37:22.901828 12548 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0002 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 4) With command 3 > root@mesos002:~/src/mesos/m1/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="cat,/etc/passwd,/etc/passwd" > I0403 21:39:35.986281 12651 scheduler.cpp:172] Version: 0.29.0 > Subscribed with ID '70abe267-a808-43fa-a1db-512af35d8ad4-0003 > task test_mesos submitted to agent 030ca997-9310-48bb-973f-d53136022537-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > 5) > root@mesos002:~/src/mesos/m2/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="echo, hello,world" > I0410 21:09:57.914875 10649 scheduler.cpp:175] Version: 0.29.0 > Subscribed with ID '11a3bd6c-be66-46d6-a5a5-8e9b635940fe-0007 > task test_mesos submitted to agent 7419ec23-763d-441f-b5d4-6d9222a1cfc9-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > From sandbox stdout: > [echo, echo, hello, world] > hello world > > 6) root@mesos002:~/src/mesos/m2/mesos/build# ./src/mesos-execute > --master=192.168.56.12:5050 --name=test_mesos --docker_image=busybox:latest > --containerizer=mesos --no-shell --command="echo, hello world" > I0410 21:09:53.783275 10581 scheduler.cpp:175] Version: 0.29.0 > Subscribed with ID '11a3bd6c-be66-46d6-a5a5-8e9b635940fe-0006 > task test_mesos submitted to agent 7419ec23-763d-441f-b5d4-6d9222a1cfc9-S0 > Received status update TASK_RUNNING for task test_mesos > Received status update TASK_FINISHED for task test_mesos > > From sandbox stdout: > [echo, echo, hello world] > hello world > > > Thanks, > > Guangya Liu > >
Re: Review Request 45926: Cleaned up flag descriptions in mesos-execute.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45926/ --- (Updated April 11, 2016, 7 p.m.) Review request for mesos, Anand Mazumdar and Joseph Wu. Repository: mesos Description --- See summary. Diffs (updated) - src/cli/execute.cpp 763dd26c359d1dd92c6e0365e4808b673efb1f40 Diff: https://reviews.apache.org/r/45926/diff/ Testing --- See the last patch in the chain: https://reviews.apache.org/r/45927/ Thanks, Alexander Rukletsov
Re: Review Request 45927: Introduced kill task delay in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/
---
(Updated April 11, 2016, 4:55 p.m.)
Review request for mesos, Anand Mazumdar and Joseph Wu.
Bugs: MESOS-5124
https://issues.apache.org/jira/browse/MESOS-5124
Repository: mesos
Description
---
Adds a flag that specifies a delay after which the task should be
killed. If set, a kill task request is scheduled to be sent after
TASK_RUNNING status update has been received. Also adds support for
TASK_KILLING capability.
Diffs
-
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45927/diff/
Testing (updated)
---
On Mac OS 10.10.4:
`make check`
Additionally manually tested `mesos-execute` with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
`./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": "2"}' --kill_after=2secs`
`./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}'
--kill_after=2secs`
Thanks,
Alexander Rukletsov
Re: Review Request 45927: Introduced kill task delay in mesos-execute.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/45927/
---
(Updated April 11, 2016, 4:54 p.m.)
Review request for mesos, Anand Mazumdar and Joseph Wu.
Bugs: MESOS-5124
https://issues.apache.org/jira/browse/MESOS-5124
Repository: mesos
Description
---
Adds a flag that specifies a delay after which the task should be
killed. If set, a kill task request is scheduled to be sent after
TASK_RUNNING status update has been received. Also adds support for
TASK_KILLING capability.
Diffs
-
src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8
Diff: https://reviews.apache.org/r/45927/diff/
Testing (updated)
---
On Mac OS 10.10.4:
`make check`
Additionally manually tested `mesos-execute` with both responsive and
unresponsive (https://github.com/rukletsov/unresponsive-process) tasks:
`./src/mesos-execute --master=127.0.0.1:5050 --name=test --command="sleep 10"
--env='{"GLOG_v": 2}' --kill_after=2secs`
`./src/mesos-execute --master=127.0.0.1:5050 --name=test
--command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": 2}'
--kill_after=2secs`
Thanks,
Alexander Rukletsov
Re: Review Request 44654: Fixed hard-coded executor shutdown grace period in executor library.
> On March 15, 2016, 6:39 p.m., Ben Mahler wrote: > > src/exec/exec.cpp, line 113 > > <https://reviews.apache.org/r/44654/diff/4/?file=1299793#file1299793line113> > > > > If you'd like to add the `private` qualifier, why isn't `kill` left as > > protected? I do not know what is our general guideline here. I think we are inconsitent about access modifiers; I've checked `AwaitProcess`, `CollectProcess`, `ReaperProcess`, `SequenceProcess`. Here I followed this sentence from the Google style guide: "Limit the use of protected to those member functions that might need to be accessed from subclasses. Note that data members should be private." I left `kill` as protected because that was the original intention of the author. I'm fine with making it private, because it's unlikely we are going to subclass it. However, it will be inconsistent to what we have in `ShutdownProcess` in "executor/executor.cpp". Moreover, I think we should factor out `ShutdownProcess` because it is already used in two places. I suggest to leave the code consistent for now and change to private when we refactor. Does it make sense? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44654/#review123713 ------- On March 15, 2016, 2:15 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44654/ > --- > > (Updated March 15, 2016, 2:15 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4911 > https://issues.apache.org/jira/browse/MESOS-4911 > > > Repository: mesos > > > Description > --- > > Executor shutdown grace period, which configured on the agent, is > propagated to executors via the `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD` > environment variable. The executor library uses this timeout to delay > the hard shutdown of the related executor. > > > Diffs > - > > src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 > > Diff: https://reviews.apache.org/r/44654/diff/ > > > Testing > --- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
Review Request 44991: Enabled mocking on `TestContainerizer::destroy`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44991/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary. Diffs - src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc src/tests/containerizer.cpp c6772ce5908edaab6c3189a65e8446217d1c7c27 Diff: https://reviews.apache.org/r/44991/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44854: Added validation for executor's shutdown grace period.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44854/ --- (Updated March 17, 2016, 11:34 p.m.) Review request for mesos and Ben Mahler. Bugs: MESOS-4949 https://issues.apache.org/jira/browse/MESOS-4949 Repository: mesos Description --- See summary. Diffs (updated) - src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca Diff: https://reviews.apache.org/r/44854/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Review Request 45040: Added a test for task's kill policy.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45040/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-4909 https://issues.apache.org/jira/browse/MESOS-4909 Repository: mesos Description --- See summary. Diffs - src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 Diff: https://reviews.apache.org/r/45040/diff/ Testing --- On Mac OS 10.104: `make check` `GTEST_FILTER="*TaskKillPolicy*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` Thanks, Alexander Rukletsov
Review Request 44992: Reordered function declarations in `TestContainerizer`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44992/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- The common pattern is to follow the order in the parent class. Diffs - src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc Diff: https://reviews.apache.org/r/44992/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Re: Review Request 44707: Added validation for task's kill policy.
> On March 15, 2016, 8:35 p.m., Ben Mahler wrote: > > Could you add a corresponding test? Having it in the header should make > > this really easy :) Sure, but I'll write a more complex test than just unit testing the validation function. - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44707/#review123743 --- On March 15, 2016, 3:47 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44707/ > --- > > (Updated March 15, 2016, 3:47 p.m.) > > > Review request for mesos and Ben Mahler. > > > Bugs: MESOS-4909 > https://issues.apache.org/jira/browse/MESOS-4909 > > > Repository: mesos > > > Description > --- > > See summary. > > > Diffs > - > > src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b > src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca > > Diff: https://reviews.apache.org/r/44707/diff/ > > > Testing > --- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
Review Request 45039: Updated the scheduler `launchTasks()` comment.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45039/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary, Diffs - include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 Diff: https://reviews.apache.org/r/45039/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
Re: Review Request 44655: Made `shutdown_grace_period` configurable in `ExecutorInfo`.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44655/ --- (Updated March 17, 2016, 11:33 p.m.) Review request for mesos, Ben Mahler and Gilbert Song. Bugs: MESOS-4949 https://issues.apache.org/jira/browse/MESOS-4949 Repository: mesos Description --- If `ExecutorInfo.shutdown_grace_period` is set, the executor driver uses it, otherwise it falls back to the environment variable `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`. Diffs (updated) - docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 include/mesos/executor/executor.proto ae211194a44e0bf2fadc79e833881e45ea3eb2c2 include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b include/mesos/v1/executor/executor.proto 36a2b3f9bc3aaa524f655b9e686a6d33512e6aaa include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b src/slave/containerizer/containerizer.cpp f6fc7863d0c215611f170dc0c89aa229407b5137 src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 Diff: https://reviews.apache.org/r/44655/diff/ Testing --- The complete chain was tested. See https://reviews.apache.org/r/44662/. Thanks, Alexander Rukletsov
Review Request 44994: Added a test for executor shutdown grace period.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44994/ --- Review request for mesos and Ben Mahler. Bugs: MESOS-4949 https://issues.apache.org/jira/browse/MESOS-4949 Repository: mesos Description --- Added a test for executor shutdown grace period. Diffs - src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 Diff: https://reviews.apache.org/r/44994/diff/ Testing --- On Mac OS 10.10.4: `make check` `GLOG_v=2 GTEST_FILTER="*SlaveTest*" ./bin/mesos-tests.sh --gtest_repeat=100 --gtest_break_on_failure` Thanks, Alexander Rukletsov
Re: Review Request 44661: Deprecated the `docker_stop_timeout` flag.
> On March 15, 2016, 11:34 p.m., Ben Mahler wrote: > > Looking pretty good. Would be great to have a CHANGELOG update here that > > outlines the deprecation and what we're advising users to do in each > > version. I'll update the changelog in the next patch, hope this is fine. > On March 15, 2016, 11:34 p.m., Ben Mahler wrote: > > src/docker/executor.hpp, line 74 > > <https://reviews.apache.org/r/44661/diff/4/?file=1299831#file1299831line74> > > > > Just to be clear, is the intention to remove this in 0.30? If so, can > > you say that explicitly? Or are we giving time for users to set kill > > policies and planning to remove it 6 months from 0.29.0? I think our *current* strategy is to remove in 6 months. That's why I put the version when we started the deprecation cycle. > On March 15, 2016, 11:34 p.m., Ben Mahler wrote: > > src/slave/flags.cpp, line 532 > > <https://reviews.apache.org/r/44661/diff/4/?file=1299834#file1299834line532> > > > > poly? Yeah, a small pony sneaked in : ). > On March 15, 2016, 11:34 p.m., Ben Mahler wrote: > > src/slave/containerizer/docker.cpp, line 967 > > <https://reviews.apache.org/r/44661/diff/4/?file=1299832#file1299832line967> > > > > Hm.. I'm a bit confused about the deprecation taking place here. Could > > you outline the steps in this description? Specifically, what does the > > 0.28, 0.29, and final versions look like and what are we advising users to > > do in each version? > > > > This information would be great to have in the CHANGELOG deprecation > > message I suggested above. I think we can remove this timeout now, but it's hard to say whether there are any users relying on the unfortunate fact that the containerizer does not kill immediately. Hence I decided to remove it via a deprecation cycle. On the other side, deprecating a bug is a bit strange. What do you think? - Alexander --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44661/#review123787 --- On March 15, 2016, 2:28 p.m., Alexander Rukletsov wrote: > > --- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44661/ > --- > > (Updated March 15, 2016, 2:28 p.m.) > > > Review request for mesos, Ben Mahler and Timothy Chen. > > > Bugs: MESOS-4910 > https://issues.apache.org/jira/browse/MESOS-4910 > > > Repository: mesos > > > Description > --- > > Instead, a combination of `executor_shutdown_grace_period` > agent flag and task kill policies should be used. > > > Diffs > - > > docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b > src/docker/executor.hpp abbc419533ab40312e917931a2fc2ce78b38da41 > src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 > src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 > src/slave/flags.cpp 4d10818105627738e258116647ccada374e3d7b9 > > Diff: https://reviews.apache.org/r/44661/diff/ > > > Testing > --- > > The complete chain was tested. See https://reviews.apache.org/r/44662/. > > > Thanks, > > Alexander Rukletsov > >
Re: Review Request 44709: Allowed unknown flags in command and docker executors.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44709/ --- (Updated March 18, 2016, 5:24 p.m.) Review request for mesos, Ben Mahler and Jie Yu. Repository: mesos Description --- We allow unknown flags to ensure that when a new flag is added in Mesos version N, agent of version N works with the executor of version N-1, i.e. the latter does not segfault when observing a newly introduced flag. Diffs (updated) - src/docker/executor.cpp afc769d0887e3842106e4c350e94c95c8ffc085e src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e Diff: https://reviews.apache.org/r/44709/diff/ Testing --- `make check` on Mac OS 10.10.4 Thanks, Alexander Rukletsov
Re: Review Request 45039: Updated the scheduler `launchTasks()` comment.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45039/ --- (Updated March 21, 2016, 9:24 p.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- See summary, Diffs (updated) - docs/app-framework-development-guide.md 1d8bebde67f69fd414509b8861571137d3569b46 include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 src/java/src/org/apache/mesos/SchedulerDriver.java bf866f5ebece2505eaa27bf39a1382cd1a2a069a src/python/interface/src/mesos/interface/__init__.py 232890daa6d222ae1c86906bbc484c8e635c4eb7 Diff: https://reviews.apache.org/r/45039/diff/ Testing --- None: not a functional change. Thanks, Alexander Rukletsov
