Re: Review Request 35291: Fixed some typos
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35291/#review87386 --- Patch looks great! Reviews applied: [35291] All tests passed. - Mesos ReviewBot On June 10, 2015, 10:20 a.m., Aditi Dixit wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35291/ --- (Updated June 10, 2015, 10:20 a.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2835 https://issues.apache.org/jira/browse/MESOS-2835 Repository: mesos Description --- Fixed some typos Diffs - src/authentication/cram_md5/auxprop.cpp 4cabaa303e19a9f9a42d1412520f5440f1949a5d src/cli/execute.cpp b387b64e7ed88ac9626fe09ced0dceb807b584bd src/common/resources.cpp 01c79fbbf16bcbdc7a25953c49107863fce3e87f src/common/values.cpp 597c4520ebc8d31a606f433593ae41c003683eb3 src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac src/tests/resources_tests.cpp 4952e1b5409103087ab7d7ad91bf907515d3c567 src/tests/status_update_manager_tests.cpp 36dab4285531634448534cb5612d3b11ff37625c Diff: https://reviews.apache.org/r/35291/diff/ Testing --- Thanks, Aditi Dixit
Review Request 35285: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35285/ --- Review request for mesos. Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos) Diffs - src/log/catchup.cpp f7afc38916d0a7e57cdecb0da7ccb3901e726b90 src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 Diff: https://reviews.apache.org/r/35285/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 35285: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35285/ --- (Updated June 10, 2015, 8:13 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2800 https://issues.apache.org/jira/browse/MESOS-2800 Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos) Diffs - src/log/catchup.cpp f7afc38916d0a7e57cdecb0da7ccb3901e726b90 src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 Diff: https://reviews.apache.org/r/35285/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 35286: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (libprocess)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35286/ --- (Updated June 10, 2015, 8:12 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2800 https://issues.apache.org/jira/browse/MESOS-2800 Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (libprocess) Diffs - 3rdparty/libprocess/src/process.cpp aadd7bb0ae12b93336900c76d8d5aaa4421ea198 3rdparty/libprocess/src/subprocess.cpp f41f5e2a34788e31749eb996c8ab38ea45989068 Diff: https://reviews.apache.org/r/35286/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 6:42 p.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. Niklas Nielsen wrote: We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? Marco Massenzio wrote: Hey Vinod - as Niklas pointed out, we have invested a significant amount of time on this one, including the manual testing I've done (as summarized on MESOS-2304) and I'd be really reluctant to start again from scratch. As I don't really think there is any semantic difference; my approach does not introduce any performance penalty (in fact, I believe it may even be better than a 'generic' one); and, finally, that this does not impact the readability of the code, we should commit the code 'as is' (with your suggestions below) and move on. There are a ton of features and work to do, and I'd like us to focus on important issues. Vinod Kone wrote: Marco. I appreciate that you invested significant effort to making sure the backwards compatibility story is airtight, but I urge you to spend some extra cycles to simplify the code and avoid tech debt. I honestly don't think it would take too much time to simplify this. I would really like to understand what's the most time consuming part here? The compatibility testing? Can you automate it with niklas's compatibility script? As an aside, going through earlier reviews I saw that BenH made similar comments but it got sidetracked with deprecating ip. Also, my fault for not jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I would be happy to shepherd this change for you going forward. Marco Massenzio wrote: The upgrade/change of MasterInfo is tracked in https://issues.apache.org/jira/browse/MESOS-2736 which I believe is a different topic than what is addressed here (which is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll update this patch description in a sec). In any event, as `ip` is a required int32 field, we MUST support it, no matter what - this patch does this, correctly, without introducing tech debt (I don't see where I'm doing that). Thanks in advance for your understanding. Vinod Kone wrote: My suggestion is to support current ip field by keeping it asis, i.e., let it be a number in json string. The reasoning is that, if we end up deciding that the easiest way to support ipv6 is by having a string field in the protobuf, then we would've 2 redundant string representations of the ip address in the resulting json. The tech debt part i was referring to was that, now there is are a set of custom functions for protobuf - json conversions for this protobuf which need to be maintained (while model() is not bad, parse() is doing a lot of work and seems to have written without the knowledge that there is an existing generic parse function IIRC?). For example, if someone adds new fields to MasterInfo they have to come and update these functions. Not to mention, you have added a bunch of tests to test the custom parsing logic, which could likely be eliminated if you can reuse the existing generic functions. Feel free to ping me on IRC if you want to discuss further. Folks, let's leverage what we have here as it's pretty clear from this discussion that we haven't figured out the answer yet for deprecating the 'ip' field and adding an IPv6 supported 'address' field or something similar. It's not worth rewriting this from scratch so instead let's do the next best thing (as we have other places too) and leave some great comments and TODOs around why the current approach was taken and what are the next steps to fix it. I think we're all on the same page that not having to write our own 'model' and 'parse' functions or have the JSON types differ from the protobuf types is the preferred approach, but given we're going to be deprecating the 'ip' field all together we can simply remove it completely (after making it optional) while simultaneously introducing the new field. It'll mean we have to keep the custom 'model' and 'parse' functions around a bit longer (while the 'ip' field still exists), but provided they're clearly documented this should be very minor and manageable. Let's keep the cadence going please! - Benjamin --- This is an automatically generated e-mail. To reply,
Re: Review Request 35287: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (stout)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35287/ --- (Updated June 10, 2015, 8:11 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2800 https://issues.apache.org/jira/browse/MESOS-2800 Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (stout) Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 8d5217a699a302cc9ebb2aa10d74cced7eb2b3d9 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp c8d30d8c193eb14f7accfde4fe02ce0710cd1817 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp f1ae80ab881ccfcefda69d5bfee2d969d171f1b9 Diff: https://reviews.apache.org/r/35287/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 35285: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35285/ --- (Updated June 10, 2015, 8:08 a.m.) Review request for mesos and Joris Van Remoortere. Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (mesos) Diffs (updated) - src/log/catchup.cpp f7afc38916d0a7e57cdecb0da7ccb3901e726b90 src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 src/master/master.hpp af83d3e82d2c161b3cc4583e78a8cbbd2f9a4064 src/slave/containerizer/containerizer.cpp e995ce602261c18373ac09c823638c4a252cca86 src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 Diff: https://reviews.apache.org/r/35285/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 35287: Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (stout)
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35287/ --- (Updated June 10, 2015, 8:10 a.m.) Review request for mesos and Joris Van Remoortere. Bugs: MESOS-2800 https://issues.apache.org/jira/browse/MESOS-2800 Repository: mesos Description --- Rename OptionT::get(const T _t) to getOrElse() and refactor original functions (stout) Diffs - 3rdparty/libprocess/3rdparty/stout/include/stout/option.hpp 8d5217a699a302cc9ebb2aa10d74cced7eb2b3d9 3rdparty/libprocess/3rdparty/stout/include/stout/os/osx.hpp c8d30d8c193eb14f7accfde4fe02ce0710cd1817 3rdparty/libprocess/3rdparty/stout/tests/option_tests.cpp f1ae80ab881ccfcefda69d5bfee2d969d171f1b9 Diff: https://reviews.apache.org/r/35287/diff/ Testing --- make check Thanks, Mark Wang
Re: Review Request 35279: Added QoS Controller module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35279/#review87402 --- Ship it! Ship It! - Bartek Plotka On June 10, 2015, 1:09 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35279/ --- (Updated June 10, 2015, 1:09 a.m.) Review request for mesos, Ben Mahler, Bartek Plotka, and Jie Yu. Bugs: MESOS-2703 https://issues.apache.org/jira/browse/MESOS-2703 Repository: mesos Description --- See summary. Diffs - include/mesos/module/qos_controller.hpp PRE-CREATION src/Makefile.am 9c6b52a9991a6bebb759e6a307c0593180507477 src/module/manager.cpp c6de654ec6188665f20bf75f079bc95f378b6a3a Diff: https://reviews.apache.org/r/35279/diff/ Testing --- make check Thanks, Niklas Nielsen
Re: Review Request 35280: Added Test QoS Controller module
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35280/#review87405 --- src/examples/test_qos_controller_module.cpp https://reviews.apache.org/r/35280/#comment139734 s/Controller module/Controller module/ (kill one space) - Bartek Plotka On June 10, 2015, 1:09 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35280/ --- (Updated June 10, 2015, 1:09 a.m.) Review request for mesos, Ben Mahler, Bartek Plotka, and Jie Yu. Bugs: MESOS-2703 https://issues.apache.org/jira/browse/MESOS-2703 Repository: mesos Description --- See summary. Diffs - src/Makefile.am 9c6b52a9991a6bebb759e6a307c0593180507477 src/examples/test_qos_controller_module.cpp PRE-CREATION Diff: https://reviews.apache.org/r/35280/diff/ Testing --- make check. Thanks, Niklas Nielsen
Re: Review Request 35278: Added missing flag initialization for qos controller.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35278/#review87401 --- Ship it! src/slave/flags.cpp https://reviews.apache.org/r/35278/#comment139731 Could you change s/resource estimator/Resource Estimator/ to be compliant with QoS Controller name? If you are Upper casing names, you can do it for RE as well.. (: - Bartek Plotka On June 10, 2015, 1:09 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35278/ --- (Updated June 10, 2015, 1:09 a.m.) Review request for mesos, Ben Mahler, Bartek Plotka, and Jie Yu. Bugs: MESOS-2703 https://issues.apache.org/jira/browse/MESOS-2703 Repository: mesos Description --- Missed this initialization during a rebase during the qos controller patch set. Diffs - src/slave/flags.cpp 99142fb1cf9d1978df86fdbf612e656b8fe852a6 Diff: https://reviews.apache.org/r/35278/diff/ Testing --- make check Ran ./bin/mesos-slave.sh --help and confirmed flag appearing. Thanks, Niklas Nielsen
Re: Review Request 35118: Made updateSlave() update its 'totalResources'.
On June 9, 2015, 10:36 a.m., Niklas Nielsen wrote: src/master/master.cpp, lines 3513-3514 https://reviews.apache.org/r/35118/diff/1/?file=980131#file980131line3513 Does it make sense to point to some documentation (if it exists already, or inline) about how this resource math will work? Jiang Yan Xu wrote: Can you suggest something here? FWIW I also updated the comment on `totalResources`: ``` // The current total resources of the slave. Note that this is // different from 'info.resources()' because this also considers // operations (e.g., CREATE, RESERVE) that have been applied and // includes revocable resources as well. Resources totalResources; ``` Ben Mahler wrote: Given the check above, how about calling .revocable to make it symmetric: ``` slave-totalResources -= slave-totalResources.revocable(); slave-totalResources += oversubscribedResources.revocable(); ``` Should be a bit clearer to the reader that what we're doing here is updating the revocable resources, without needing a big comment. SGTM. Will do. - Jiang Yan --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35118/#review87234 --- On June 5, 2015, 2:09 p.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35118/ --- (Updated June 5, 2015, 2:09 p.m.) Review request for mesos and Vinod Kone. Bugs: MESOS-2776 https://issues.apache.org/jira/browse/MESOS-2776 Repository: mesos Description --- - This way Master::Slave::totalResources includes revocable resources, which we need for metrics for revocable resources. - Changed updateSlave() argument to use `const Resources oversubscribedResources` instead of `const std::vectorResource oversubscribedResources` because `Resources` provides convenience methods such as `revocable()`. Diffs - src/master/master.hpp deeb0d8c87a13315206556e1d0974cdd13e8224f src/master/master.cpp be0db42da3c59761aa154439653d715556465256 Diff: https://reviews.apache.org/r/35118/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 35281: Added QoS module loader to ::create() factory.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35281/#review87421 --- Ship it! src/slave/qos_controller.cpp https://reviews.apache.org/r/35281/#comment139742 Reorder. - Bartek Plotka On June 10, 2015, 1:09 a.m., Niklas Nielsen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35281/ --- (Updated June 10, 2015, 1:09 a.m.) Review request for mesos, Ben Mahler, Bartek Plotka, and Jie Yu. Bugs: MESOS-2703 https://issues.apache.org/jira/browse/MESOS-2703 Repository: mesos Description --- See summary. Diffs - src/slave/qos_controller.cpp 81c4b3e658902be0438f42d9e86911e424828a73 Diff: https://reviews.apache.org/r/35281/diff/ Testing --- make check Tested new example module manually, since we don't have a good type param'ed test for resource estimator and qos controller yet. Thanks, Niklas Nielsen
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
On June 3, 2015, 6:08 p.m., Ben Mahler wrote: Can we make the test a unit test? Looks like we could pull up '`validateResources`' to make this unit-testable? Chatting with Jie, we should probably place it in an `'internal::task'` namespace towards the bottom of the header, since it's only for testing purposes and we want to make it clear which validation functions are expected to be used by the master. done. On June 3, 2015, 6:08 p.m., Ben Mahler wrote: src/master/validation.cpp, lines 321-326 https://reviews.apache.org/r/34910/diff/1/?file=975981#file975981line321 Are we otherwise allowing mixing within a resource? Looking at the existing cpushare.cpp logic, it seems that we have a binary decision: (1) If any of the cpus are revocable, *all* cpus are treated revocable for isolation purposes. (2) Otherwise, non-revocable. Is the idea here that the mixing that (1) allows enables a framework to reduce the risk of revocation? Maybe a TODO here and/or on the cpushare code since it's not that difficult to change the cpushare implementation to allow transitioning between revocable and non-revocable after the container is launched? per the latest discussion, we are not allowing mixing of resources (revocable and non-revocable) for any given type (e.g., cpus). On June 3, 2015, 6:08 p.m., Ben Mahler wrote: src/tests/master_validation_tests.cpp, line 1105 https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1105 its :) N/A. On June 3, 2015, 6:08 p.m., Ben Mahler wrote: src/tests/master_validation_tests.cpp, lines 1136-1137 https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1136 I find it nice to reduce jaggedness on comments, e.g.: ``` // Create a task that uses revocable resources // while it's executor does not. ``` I think most of our code just wraps the comments at 70. On June 3, 2015, 6:08 p.m., Ben Mahler wrote: src/tests/master_validation_tests.cpp, lines 1104-1105 https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1104 From this comment, I expected this test to be testing that a task with revocable resources is allowed when the executor has revocable resources, but it's only testing the opposite condition. The unit test would make it easier to test both cases, yes? added tests for both cases. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/#review86449 --- On June 1, 2015, 11:11 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 1, 2015, 11:11 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs - src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 35305: Added a SlaveTest to caputure the case when containerizer usage fails.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35305/#review87443 --- Patch looks great! Reviews applied: [35259, 35260, 35264, 35305] All tests passed. - Mesos ReviewBot On June 10, 2015, 6:30 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35305/ --- (Updated June 10, 2015, 6:30 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a SlaveTest to caputure the case when containerizer usage fails. Diffs - src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 Diff: https://reviews.apache.org/r/35305/diff/ Testing --- make check Thanks, Jie Yu
Review Request 35312: Minor cleanup to master/slave metric logic.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35312/ --- Review request for mesos and Jiang Yan Xu. Repository: mesos Description --- No functional changes, some cleanups along the way to r/35313 Diffs - src/master/master.cpp e3b7081e252b2b1d582e6c6c88a433217a31fa3e src/master/metrics.hpp 3d389e68f22e3d1e00bde0db4e25f897c79a2316 src/master/metrics.cpp d2489c8897fb2f2f21f021ac5e7a2ada7997ea06 src/slave/metrics.cpp 7a31ce7e32c1fd61256927cd37d84a646bf5dbda src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac Diff: https://reviews.apache.org/r/35312/diff/ Testing --- make check Thanks, Ben Mahler
Review Request 35305: Added a SlaveTest to caputure the case when containerizer usage fails.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35305/ --- Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a SlaveTest to caputure the case when containerizer usage fails. Diffs - src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 Diff: https://reviews.apache.org/r/35305/diff/ Testing --- make check Thanks, Jie Yu
Review Request 35309: Added Resources::get() and Resources::names().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35309/ --- Review request for mesos, Ben Mahler and Jie Yu. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Need this for the subsequent review. Diffs - include/mesos/resources.hpp 6feb1be5a1463ae1a2d932b2ff696f79c7d9afb5 src/common/resources.cpp 01c79fbbf16bcbdc7a25953c49107863fce3e87f src/tests/resources_tests.cpp 4952e1b5409103087ab7d7ad91bf907515d3c567 Diff: https://reviews.apache.org/r/35309/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
On June 3, 2015, 6:34 p.m., Ben Mahler wrote: Ditto, can we roll it in to a unit test of '`validateResources`'? I really want to have an end to end test to make sure that the wiring is all correct. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/#review86457 --- On June 1, 2015, 11:15 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 1, 2015, 11:15 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs - src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/slave/slave.cpp fdaaea42ca9afd4af197f89edb31678723e9acbc src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
On June 3, 2015, 8:33 p.m., Niklas Nielsen wrote: src/tests/oversubscription_tests.cpp, line 185 https://reviews.apache.org/r/34911/diff/1/?file=975987#file975987line185 Do you only want to start the executor on oversubscribed resources or split it between task and executor? (Which I guess should be the common use case) Now both task and executor should use revocable resources. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/#review86477 --- On June 1, 2015, 11:15 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 1, 2015, 11:15 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs - src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/slave/slave.cpp fdaaea42ca9afd4af197f89edb31678723e9acbc src/tests/mesos.hpp ac986a0687a576a0bd5693c82b3c7d543aaa956b src/tests/mesos.cpp 1d5639c85517229f3396b40f2d8bd421b2ed7325 src/tests/oversubscription_tests.cpp ea5857cb579aa904fd05530684bdde51a0b3f27f Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 10, 2015, 7:11 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Changes --- updated the test to work with the latest invariant. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs (updated) - src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 35320: Fixed a bug in the fixed resource estimator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35320/ --- Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2791 https://issues.apache.org/jira/browse/MESOS-2791 Repository: mesos Description --- Fixed a bug in the fixed resource estimator. Diffs - src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 Diff: https://reviews.apache.org/r/35320/diff/ Testing --- make check Thanks, Jie Yu
Review Request 35321: Implemented the fixed resource estimator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35321/ --- Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2791 https://issues.apache.org/jira/browse/MESOS-2791 Repository: mesos Description --- Implemented the fixed resource estimator. Diffs - src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/35321/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35264: Added a slave integration test in MonitorTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/ --- (Updated June 10, 2015, 6:30 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Changes --- Addressed review comments. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a slave integration test in MonitorTest. This test verifies the wiring between resource monitor and the slave. Diffs (updated) - src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35264/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35260: Refactored the ResourceMonitor to get statistics from the Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35260/ --- (Updated June 10, 2015, 6:29 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Changes --- Addressed review comments. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Refactored the ResourceMonitor to get statistics from the Slave. 1) Modified ResourceUsage to include allocation information (see ticket for reaons). 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics. 3) Adjusted (and simplified) the MonitorTest accordingly. Diffs (updated) - include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87 include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7 src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20 src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35260/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 30952: Adding scheduler validations to master
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30952/#review87438 --- Ship it! Ship It! - Vinod Kone On June 10, 2015, 12:08 a.m., Isabel Jimenez wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30952/ --- (Updated June 10, 2015, 12:08 a.m.) Review request for mesos, Jie Yu, Joris Van Remoortere, and Vinod Kone. Bugs: MESOS-2290 https://issues.apache.org/jira/browse/MESOS-2290 Repository: mesos-incubating Description --- There is a scheduler validation (https://github.com/apache/mesos/blob/master/src/scheduler/scheduler.cpp#L275) missing in master. See motivation on MESOS-2288. Diffs - src/master/master.cpp e91e881 src/master/validation.cpp 1793b0e src/sched/sched.cpp 8c366ec src/scheduler/scheduler.cpp f613ca4 Diff: https://reviews.apache.org/r/30952/diff/ Testing --- make check and distcheck. Tests will be added with new HTTP API endpoints tests. Thanks, Isabel Jimenez
Re: Review Request 35259: Removed unused constaints from ResourceMonitor.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35259/#review87439 --- Ship it! Ship It! - Vinod Kone On June 9, 2015, 7:50 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35259/ --- (Updated June 9, 2015, 7:50 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Removed unused constaints from ResourceMonitor. Diffs - src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7 Diff: https://reviews.apache.org/r/35259/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35247: Fixed race between EXPECT_CALL(resourceOffers, _) and driver.start() in fetcher_cache_tests.cpp.
On June 9, 2015, 6:11 p.m., Vinod Kone wrote: src/tests/fetcher_cache_tests.cpp, lines 201-207 https://reviews.apache.org/r/35247/diff/1/?file=981340#file981340line201 While this looks good as a temporary fix, what is the long term strategy here? I really don't like setting expectations in SetUp() or TearDown() because it's really hard to reason about in the individual tests. For example, why did you set expecations on registered and offers but not others? I prefer to move these expectations to tests. Also, this SetUp() is doing too much (starting slave, starting master, constructing scheduler but not starting it, setting some expectations) and there is no documentation for it! Bernd Mathiske wrote: Long term I am working on developing up stress tests for the fetcher. These are still relatively basic functionality tests so far. Yes, SetUp() and TearDown() do a lot here. Would you prefer a) inlining them or b) commenting what they do more or c) both? In my experience a) would be most consistent with existing patterns, but it makes it harder to spot what the different tests are doing besides all the code that is always the same. And the general code blowup would be rather substantial in this test series. Bernd Mathiske wrote: Maybe you meant the long term plan wrt. the code structure here. In this case, it is to create more generally applicable test pattern lego blocks that aggregate such things as in the Setup() here. These will have to be carefully selected/crafted then. i would prefer to inline them as we do everywhere else in the code base. The current abstractions are a bit hard to follow. On June 9, 2015, 6:11 p.m., Vinod Kone wrote: src/tests/fetcher_cache_tests.cpp, lines 360-361 https://reviews.apache.org/r/35247/diff/1/?file=981340#file981340line360 Why not just do AWAIT_READY(offers)? Bernd Mathiske wrote: That does not compile here, because it contains a return of type void and that does not match the return type of launch(). aah. ok. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35247/#review87235 --- On June 9, 2015, 9:32 a.m., Bernd Mathiske wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35247/ --- (Updated June 9, 2015, 9:32 a.m.) Review request for mesos, Benjamin Hindman, Till Toenshoff, and Vinod Kone. Bugs: MESOS-2815, MESOS-2829 and MESOS-2831 https://issues.apache.org/jira/browse/MESOS-2815 https://issues.apache.org/jira/browse/MESOS-2829 https://issues.apache.org/jira/browse/MESOS-2831 Repository: mesos Description --- Fixed race between EXPECT_CALL(resourceOffers, _) and driver.start() in fetcher_cache_tests.cpp. Installed a default response that provides teporary cover for this mocked method until we install more interesting callbacks later. This prevents gmock from complaining about an uninteresting gmock call, which led to a variety of tests failing due to offers not getting through subsequently. All fetcher cache tests have been affected by this race and should be fixed in this regard now. (Also fixed some typos.) Diffs - src/tests/fetcher_cache_tests.cpp cbd44b98d19953d174fac977f509d4900a37481f Diff: https://reviews.apache.org/r/35247/diff/ Testing --- make check Thanks, Bernd Mathiske
Re: Review Request 35313: Added slave metrics for revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35313/#review87462 --- Ship it! src/slave/slave.cpp https://reviews.apache.org/r/35313/#comment139774 why the temporary? - Vinod Kone On June 10, 2015, 7:50 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35313/ --- (Updated June 10, 2015, 7:50 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-2775 https://issues.apache.org/jira/browse/MESOS-2775 Repository: mesos Description --- Same approach as done in [r/35119/](https://reviews.apache.org/r/35119/) for [MESOS-2776](https://issues.apache.org/jira/browse/MESOS-2776)]. Note that the existing used metric was not ignoring revocable resources. Diffs - src/slave/metrics.hpp 6af7f074d4e41225867e482241988bec3a9806e9 src/slave/metrics.cpp 7a31ce7e32c1fd61256927cd37d84a646bf5dbda src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac Diff: https://reviews.apache.org/r/35313/diff/ Testing --- Manual testing, I will follow up to get this tested within vinod/jie's integration tests. Note that in the process of testing this, I realized that the command executor leads to a mixing of revocable / non-revocable resources in the slave. Will file a ticket. Thanks, Ben Mahler
Re: Review Request 35313: Added slave metrics for revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35313/#review87474 --- Patch looks great! Reviews applied: [35312, 35313] All tests passed. - Mesos ReviewBot On June 10, 2015, 7:50 p.m., Ben Mahler wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35313/ --- (Updated June 10, 2015, 7:50 p.m.) Review request for mesos and Jiang Yan Xu. Bugs: MESOS-2775 https://issues.apache.org/jira/browse/MESOS-2775 Repository: mesos Description --- Same approach as done in [r/35119/](https://reviews.apache.org/r/35119/) for [MESOS-2776](https://issues.apache.org/jira/browse/MESOS-2776)]. Note that the existing used metric was not ignoring revocable resources. Diffs - src/slave/metrics.hpp 6af7f074d4e41225867e482241988bec3a9806e9 src/slave/metrics.cpp 7a31ce7e32c1fd61256927cd37d84a646bf5dbda src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac Diff: https://reviews.apache.org/r/35313/diff/ Testing --- Manual testing, I will follow up to get this tested within vinod/jie's integration tests. Note that in the process of testing this, I realized that the command executor leads to a mixing of revocable / non-revocable resources in the slave. Will file a ticket. Thanks, Ben Mahler
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
On June 5, 2015, 6:42 p.m., Vinod Kone wrote: I made some minor comments below but I think a better way to do this is to *not* write custom masterinfo json - protobuf converters. I prefer we just add a new optional field (say ipAddress of type string). Then you can just leverage the already existing generic protobuf - json converters. Niklas Nielsen wrote: We have been working on this for a while now and we are using JSON::Protobuf() aldready and can enhance it further with your suggestion. However, the current approach isn't semantically different from the one you suggest and we would like to move forward and make that change later. Is that OK with you? Marco Massenzio wrote: Hey Vinod - as Niklas pointed out, we have invested a significant amount of time on this one, including the manual testing I've done (as summarized on MESOS-2304) and I'd be really reluctant to start again from scratch. As I don't really think there is any semantic difference; my approach does not introduce any performance penalty (in fact, I believe it may even be better than a 'generic' one); and, finally, that this does not impact the readability of the code, we should commit the code 'as is' (with your suggestions below) and move on. There are a ton of features and work to do, and I'd like us to focus on important issues. Vinod Kone wrote: Marco. I appreciate that you invested significant effort to making sure the backwards compatibility story is airtight, but I urge you to spend some extra cycles to simplify the code and avoid tech debt. I honestly don't think it would take too much time to simplify this. I would really like to understand what's the most time consuming part here? The compatibility testing? Can you automate it with niklas's compatibility script? As an aside, going through earlier reviews I saw that BenH made similar comments but it got sidetracked with deprecating ip. Also, my fault for not jumping on this sooner, but in my defense, I wasn't marked as a reviewer. I would be happy to shepherd this change for you going forward. Marco Massenzio wrote: The upgrade/change of MasterInfo is tracked in https://issues.apache.org/jira/browse/MESOS-2736 which I believe is a different topic than what is addressed here (which is tracked here: https://issues.apache.org/jira/browse/MESOS-2807 - I'll update this patch description in a sec). In any event, as `ip` is a required int32 field, we MUST support it, no matter what - this patch does this, correctly, without introducing tech debt (I don't see where I'm doing that). Thanks in advance for your understanding. Vinod Kone wrote: My suggestion is to support current ip field by keeping it asis, i.e., let it be a number in json string. The reasoning is that, if we end up deciding that the easiest way to support ipv6 is by having a string field in the protobuf, then we would've 2 redundant string representations of the ip address in the resulting json. The tech debt part i was referring to was that, now there is are a set of custom functions for protobuf - json conversions for this protobuf which need to be maintained (while model() is not bad, parse() is doing a lot of work and seems to have written without the knowledge that there is an existing generic parse function IIRC?). For example, if someone adds new fields to MasterInfo they have to come and update these functions. Not to mention, you have added a bunch of tests to test the custom parsing logic, which could likely be eliminated if you can reuse the existing generic functions. Feel free to ping me on IRC if you want to discuss further. Benjamin Hindman wrote: Folks, let's leverage what we have here as it's pretty clear from this discussion that we haven't figured out the answer yet for deprecating the 'ip' field and adding an IPv6 supported 'address' field or something similar. It's not worth rewriting this from scratch so instead let's do the next best thing (as we have other places too) and leave some great comments and TODOs around why the current approach was taken and what are the next steps to fix it. I think we're all on the same page that not having to write our own 'model' and 'parse' functions or have the JSON types differ from the protobuf types is the preferred approach, but given we're going to be deprecating the 'ip' field all together we can simply remove it completely (after making it optional) while simultaneously introducing the new field. It'll mean we have to keep the custom 'model' and 'parse' functions around a bit longer (while the 'ip' field still exists), but provided they're clearly documented this should b e very minor and manageable. Let's keep the cadence going please! I don't follow. When we do add the 'address' field of type string in the protobuf,
Re: Review Request 35264: Added a slave integration test in MonitorTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/#review87468 --- Ship it! src/tests/monitor_tests.cpp https://reviews.apache.org/r/35264/#comment139788 Why not keep MonitorTest to be clear that these are unit tests, and introduce a MonitorIntegrationTest that inherits from MesosTest? I could imagine multiple test cases within integration, so seems better on the left hand side: MonitorIntegrationTest.RunningExecutor MonitorIntegrationTest.TerminatedExecutors Just as an example. src/tests/monitor_tests.cpp https://reviews.apache.org/r/35264/#comment139789 This case is still ok for taking a const , since it's not a temporary. src/tests/monitor_tests.cpp https://reviews.apache.org/r/35264/#comment139791 Took me awhile to figure out that this was calling process::address(). :) At least we should be adding the process:: qualifier here, no? - Ben Mahler On June 10, 2015, 6:30 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/ --- (Updated June 10, 2015, 6:30 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a slave integration test in MonitorTest. This test verifies the wiring between resource monitor and the slave. Diffs - src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35264/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/#review87465 --- Ship it! src/master/validation.hpp https://reviews.apache.org/r/34910/#comment139777 Can you instead use namespace task::internal? See my comments below. src/master/validation.cpp https://reviews.apache.org/r/34910/#comment139781 This looks wiered. Can we instead put all the validateXXX function under task::internal namespace? You can keep validateResources close to validateResourceUsage as it is right now. And this code block will look more consistent: ``` lambda::bind(internal::validateTaskID, ...), lambda::bind(internal::validateUnique...), ... ``` src/master/validation.cpp https://reviews.apache.org/r/34910/#comment139782 See my above comments. You may want to move this close to where all the task validation functions are located. src/master/validation.cpp https://reviews.apache.org/r/34910/#comment139786 Is this check redundent? Can you calculate the total first and do one check instead? src/master/validation.cpp https://reviews.apache.org/r/34910/#comment139790 I would rather swap the order of these two checks: ``` if (!resources.revocable.empty() resources.revocable() != resources) ``` I found the above more easy to read:) - Jie Yu On June 10, 2015, 7:10 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 10, 2015, 7:10 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs - src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765 src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 35321: Implemented the fixed resource estimator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35321/#review87454 --- Patch looks great! Reviews applied: [35259, 35260, 35264, 35305, 35320, 35321] All tests passed. - Mesos ReviewBot On June 10, 2015, 7:49 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35321/ --- (Updated June 10, 2015, 7:49 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2791 https://issues.apache.org/jira/browse/MESOS-2791 Repository: mesos Description --- Implemented the fixed resource estimator. Diffs - src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/35321/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35260: Refactored the ResourceMonitor to get statistics from the Slave.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/#review87459
---
Ship it!
src/slave/monitor.cpp
https://reviews.apache.org/r/35260/#comment139771
Feel free to remove this line now.
src/slave/monitor.cpp
https://reviews.apache.org/r/35260/#comment139787
Do you need to copy all the data out of the future? Why not take a const
here or just inline it in the loop:
```
foreach (const ResourceUsage::Executor executor,
future.get().executors()) {
```
I see that this is 81 characters which is probably why you pulled it out,
when we have an - operator for Future it will fit, oh well. :)
src/slave/resource_estimators/fixed.cpp
https://reviews.apache.org/r/35260/#comment139780
Whoops! Remove the reference?
src/slave/slave.cpp
https://reviews.apache.org/r/35260/#comment139776
Seems ok for now, but a bit odd that we're copying it in the capture, since
ideally we want to move.
src/slave/slave.cpp
https://reviews.apache.org/r/35260/#comment139775
Want a CHECK_EQ on the sizes?
src/slave/slave.cpp
https://reviews.apache.org/r/35260/#comment139778
I know we're inconsistent about this, but we should probably single quote
the ID here, since it's coming from the framework.
src/slave/slave.cpp
https://reviews.apache.org/r/35260/#comment139779
Trivial, but wrapping after Fix looks less jagged?
src/tests/monitor_tests.cpp
https://reviews.apache.org/r/35260/#comment139783
I think we're trying to avoid introducing more of these clauses where we
pull in all of 'process', but since we don't have namespace aliases yet, this
seems ok.
- Ben Mahler
On June 10, 2015, 6:29 p.m., Jie Yu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35260/
---
(Updated June 10, 2015, 6:29 p.m.)
Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
Bugs: MESOS-2818
https://issues.apache.org/jira/browse/MESOS-2818
Repository: mesos
Description
---
Refactored the ResourceMonitor to get statistics from the Slave.
1) Modified ResourceUsage to include allocation information (see ticket for
reaons).
2) Simplied the ResourceMonitor by taking a lambda from the slave to get
statistics.
3) Adjusted (and simplified) the MonitorTest accordingly.
Diffs
-
include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87
include/mesos/slave/resource_estimator.hpp
7f78fd86760397d5b885a2c00b41081d0b546cdd
src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7
src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c
src/slave/resource_estimators/fixed.cpp
3efa18d7e3c6ac62e67f75ea45a832f3f6349036
src/slave/resource_estimators/noop.hpp
7a44e6deefc9c1985c14d076a427aa5c654aa1bb
src/slave/resource_estimators/noop.cpp
5f135ff37e84c248ede29bbe4a7d1b8319417e20
src/slave/slave.hpp 4d2c31688b19f101ec851c0d94e7d45aa2f8a76e
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0
Diff: https://reviews.apache.org/r/35260/diff/
Testing
---
make check
Thanks,
Jie Yu
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34911/#review87473
---
Ship it!
src/slave/slave.cpp
https://reviews.apache.org/r/34911/#comment139798
I think we have streaming overload for RepeatedPtrFieldResource. So get
rid of the 'Resources' part.
src/tests/oversubscription_tests.cpp
https://reviews.apache.org/r/34911/#comment139804
Can you use `createTask` here?
```
TaskInfo task = createTask(
offer2.get()[0].slave_id(),
taskResources,
exit 1,
exec.id);
task.mutable_executor()-mutable_resources()
-CopyFrom(executorResources);
```
src/tests/oversubscription_tests.cpp
https://reviews.apache.org/r/34911/#comment139803
No need for this. You can directly use initialization list `{task}`.
- Jie Yu
On June 10, 2015, 7:11 p.m., Vinod Kone wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34911/
---
(Updated June 10, 2015, 7:11 p.m.)
Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu.
Bugs: MESOS-2753
https://issues.apache.org/jira/browse/MESOS-2753
Repository: mesos
Description
---
An end to end test that launches a revocable executor.
Note that this doesn't exercise cpu isolator because it uses the test
containerizer. Cpu isolator with revocable resources is already tested.
BenM is writing an example framework that should test this with cpu isolator.
Diffs
-
src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962
src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac
src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417
src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a
src/tests/oversubscription_tests.cpp
59cf07ef93460537ce1343793fd4a5d11d2ae242
Diff: https://reviews.apache.org/r/34911/diff/
Testing
---
make check
Thanks,
Vinod Kone
Re: Review Request 35264: Added a slave integration test in MonitorTest.
On June 10, 2015, 9:58 p.m., Ben Mahler wrote: src/tests/monitor_tests.cpp, line 243 https://reviews.apache.org/r/35264/diff/2/?file=982150#file982150line243 This case is still ok for taking a const , since it's not a temporary. Fixed. On June 10, 2015, 9:58 p.m., Ben Mahler wrote: src/tests/monitor_tests.cpp, line 58 https://reviews.apache.org/r/35264/diff/2/?file=982150#file982150line58 Why not keep MonitorTest to be clear that these are unit tests, and introduce a MonitorIntegrationTest that inherits from MesosTest? I could imagine multiple test cases within integration, so seems better on the left hand side: MonitorIntegrationTest.RunningExecutor MonitorIntegrationTest.TerminatedExecutors Just as an example. Good idea! Done. - Jie --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/#review87468 --- On June 10, 2015, 6:30 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/ --- (Updated June 10, 2015, 6:30 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a slave integration test in MonitorTest. This test verifies the wiring between resource monitor and the slave. Diffs - src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35264/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35327: Fixed a bug that allows different resources of the same name to have different types.
On June 10, 2015, 3:56 p.m., Vinod Kone wrote:
src/common/resources.cpp, lines 383-388
https://reviews.apache.org/r/35327/diff/1/?file=982274#file982274line383
No need for else if.
if () {
}
nameTypes[name] = resource.get().type();
Jiang Yan Xu wrote:
Just that the if ... else ... structure logically separates the chunk of
code that does the bookkeeping from the rest of the sequence of logic.
e.g.
```cpp
if () {
}
nameTypes[name] = resource.get().type();
resources += resource.get();
```
I could to remove the blank line between the if block and the subsquence
line like this:
```cpp
if () {
}
nameTypes[name] = resource.get().type();
```
But we generally don't do this, I think.
Oh I didn't even realize that it's an else if rather than else.
In this case `nameTypes[name] = resource.get().type();` is a (logical) noop if
`nameTypes.contains(name)` but anyways, I think in this case leaving the `else`
there is fine.
- Jiang Yan
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35327/#review87482
---
On June 10, 2015, 3:34 p.m., Jiang Yan Xu wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/35327/
---
(Updated June 10, 2015, 3:34 p.m.)
Review request for mesos and Vinod Kone.
Bugs: MESOS-2854
https://issues.apache.org/jira/browse/MESOS-2854
Repository: mesos
Description
---
See summary.
Diffs
-
src/common/resources.cpp c168bd83d0e0f07fb8f3a70114dd854cad5f5140
src/tests/resources_tests.cpp b1e4483695eda998129db2c89a9dce044607c8b0
Diff: https://reviews.apache.org/r/35327/diff/
Testing
---
make check.
Updated test `ResourcesTest.ParseError` to cover this case.
Thanks,
Jiang Yan Xu
Review Request 35333: Small fix in updateSlave() to make resource math clearer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35333/ --- Review request for mesos and Ben Mahler. Repository: mesos Description --- A follow-up on BenM's suggestion in /r/35118/. Diffs - src/master/master.cpp 95ca2e5f6c903dea7528e559f86639e78ec92b9b Diff: https://reviews.apache.org/r/35333/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 35305: Added a SlaveTest to caputure the case when containerizer usage fails.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35305/#review87501 --- Ship it! Ship It! - Ben Mahler On June 10, 2015, 6:30 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35305/ --- (Updated June 10, 2015, 6:30 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a SlaveTest to caputure the case when containerizer usage fails. Diffs - src/tests/slave_tests.cpp acae49731ff17103b529cdf828a63d9d55668549 Diff: https://reviews.apache.org/r/35305/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35330/#review87504 --- Patch looks great! Reviews applied: [35330] All tests passed. - Mesos ReviewBot On June 11, 2015, 12:04 a.m., Lily Chen wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35330/ --- (Updated June 11, 2015, 12:04 a.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2797 https://issues.apache.org/jira/browse/MESOS-2797 Repository: mesos Description --- Capped number of parallel inspect instances on a docker ps call. Diffs - src/docker/docker.hpp 7790d0ff9a6b085025f595533c5f46b702447927 src/docker/docker.cpp 71383294c6234d08b2156565b170ada329b8e30f src/slave/constants.hpp 84927e589499e989249c217db571bbeb84a88af1 Diff: https://reviews.apache.org/r/35330/diff/ Testing --- Tweaked /mesos/src/slave/containerizer/docker.cpp DockerContainerizerProcess::recover function to call a 'docker ps -a' without a prefix. Then ran 'docker run busybox' 1300 times to recreate failure, failing DockerContainerizerTest.ROOT_DOCKER_Recover and DockerContainerizerTest.ROOT_DOCKER_SkipRecoverNonDocker, also causing slave to fail when launched. Also tested fix after clearing 'docker ps -a' log and with 300 instances in 'docker ps -a' log to make sure original functionality not broken. Thanks, Lily Chen
Re: Review Request 35260: Refactored the ResourceMonitor to get statistics from the Slave.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35260/ --- (Updated June 10, 2015, 11:13 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Changes --- Review comments. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Refactored the ResourceMonitor to get statistics from the Slave. 1) Modified ResourceUsage to include allocation information (see ticket for reaons). 2) Simplied the ResourceMonitor by taking a lambda from the slave to get statistics. 3) Adjusted (and simplified) the MonitorTest accordingly. Diffs (updated) - include/mesos/mesos.proto 7457ff11f6a55099ccb95beb2f0ccb9a2f7ccd87 include/mesos/slave/resource_estimator.hpp 7f78fd86760397d5b885a2c00b41081d0b546cdd src/slave/monitor.hpp bee91ba143c26059fc8badf56beccb68c6556cb7 src/slave/monitor.cpp 8f7ff63b4daf0286d4cf912e2f3d2d1b68caab1c src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 src/slave/resource_estimators/noop.hpp 7a44e6deefc9c1985c14d076a427aa5c654aa1bb src/slave/resource_estimators/noop.cpp 5f135ff37e84c248ede29bbe4a7d1b8319417e20 src/slave/slave.hpp 98c64f62f60c286e42379b2151321b190c4410b8 src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836 src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35260/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35264: Added a slave integration test in MonitorTest.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35264/ --- (Updated June 10, 2015, 11:13 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Changes --- Review comments. Bugs: MESOS-2818 https://issues.apache.org/jira/browse/MESOS-2818 Repository: mesos Description --- Added a slave integration test in MonitorTest. This test verifies the wiring between resource monitor and the slave. Diffs (updated) - src/tests/monitor_tests.cpp 6de8b1f65843fd7b852dfa69627a1c435b482fe0 Diff: https://reviews.apache.org/r/35264/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 10, 2015, 11:57 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Changes --- rebased. NNFR. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs (updated) - src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836 src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 10, 2015, 11:56 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Changes --- rebased. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs (updated) - src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765 src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836 src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 35330: Capped number of parallel inspect instances on a docker ps call.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35330/ --- Review request for mesos and Timothy Chen. Bugs: MESOS-2797 https://issues.apache.org/jira/browse/MESOS-2797 Repository: mesos Description --- Capped number of parallel inspect instances on a docker ps call. Diffs - src/docker/docker.hpp 7790d0ff9a6b085025f595533c5f46b702447927 src/docker/docker.cpp 71383294c6234d08b2156565b170ada329b8e30f src/slave/constants.hpp 84927e589499e989249c217db571bbeb84a88af1 Diff: https://reviews.apache.org/r/35330/diff/ Testing --- Tweaked /mesos/src/slave/containerizer/docker.cpp DockerContainerizerProcess::recover function to call a 'docker ps -a' without a prefix. Then ran 'docker run busybox' 1300 times to recreate failure, failing two docker tests. Also ran fix after clearing 'docker ps -a' log and with 300 instances in 'docker ps -a' log to make sure original functionality not broken. Thanks, Lily Chen
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 10, 2015, 11:18 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Changes --- jie's. NNFR. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs (updated) - src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765 src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp bb9dd07a9c5ce0d82016809d9eb647b2c64b2d8f src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
On June 10, 2015, 10:03 p.m., Jie Yu wrote: src/master/validation.cpp, lines 714-721 https://reviews.apache.org/r/34910/diff/2/?file=982185#file982185line714 Is this check redundent? Can you calculate the total first and do one check instead? I originally just had one. But opted to split the check for a better/concise error message. I'll just update the error message and have just one check. - Vinod --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/#review87465 --- On June 10, 2015, 7:10 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 10, 2015, 7:10 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs - src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765 src/master/validation.cpp 1793b0e16fc36ead147880ee06cda40b23fa0962 src/slave/slave.cpp 98036b2d5f2c765aef4a416c3cbc082df77ab3ac src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 10, 2015, 11:18 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Changes --- jie's. NNFR. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs (updated) - src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp bb9dd07a9c5ce0d82016809d9eb647b2c64b2d8f src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 35309: Added Resources::get() and Resources::names().
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35309/#review87489 --- Ship it! I can use the code in this review if we get it committed soon. - Jiang Yan Xu On June 10, 2015, 12:08 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35309/ --- (Updated June 10, 2015, 12:08 p.m.) Review request for mesos, Ben Mahler and Jie Yu. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Need this for the subsequent review. Diffs - include/mesos/resources.hpp 6feb1be5a1463ae1a2d932b2ff696f79c7d9afb5 src/common/resources.cpp 01c79fbbf16bcbdc7a25953c49107863fce3e87f src/tests/resources_tests.cpp 4952e1b5409103087ab7d7ad91bf907515d3c567 Diff: https://reviews.apache.org/r/35309/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 35332: Add a test case for egress flow ID recovery
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35332/ --- Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu. Bugs: MESOS-2799 https://issues.apache.org/jira/browse/MESOS-2799 Repository: mesos Description --- Add a test case for egress flow ID recovery Diffs - src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35332/diff/ Testing --- make check Thanks, Cong Wang
Re: Review Request 34911: Added a test for launching an executor on revocable resources.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/#review87490 --- Bad patch! Reviews applied: [35309, 34910] Failed command: ./support/apply-review.sh -n -r 34910 Error: 2015-06-10 23:34:43 URL:https://reviews.apache.org/r/34910/diff/raw/ [8902/8902] - 34910.patch [1] error: patch failed: src/slave/slave.cpp:3098 error: src/slave/slave.cpp: patch does not apply Failed to apply patch - Mesos ReviewBot On June 10, 2015, 11:18 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34911/ --- (Updated June 10, 2015, 11:18 p.m.) Review request for mesos, Ben Mahler, Ian Downes, and Jie Yu. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- An end to end test that launches a revocable executor. Note that this doesn't exercise cpu isolator because it uses the test containerizer. Cpu isolator with revocable resources is already tested. BenM is writing an example framework that should test this with cpu isolator. Diffs - src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp bb9dd07a9c5ce0d82016809d9eb647b2c64b2d8f src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a src/tests/oversubscription_tests.cpp 59cf07ef93460537ce1343793fd4a5d11d2ae242 Diff: https://reviews.apache.org/r/34911/diff/ Testing --- make check Thanks, Vinod Kone
Review Request 35331: Add a test case for egress flow ID
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35331/ --- Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu. Bugs: MESOS-2799 https://issues.apache.org/jira/browse/MESOS-2799 Repository: mesos Description --- Add a test case for egress flow ID uniqueness Diffs - src/slave/containerizer/isolators/network/port_mapping.hpp 4c719b186b519fad0c3869dbdae8b60c3a2c20cc src/slave/containerizer/isolators/network/port_mapping.cpp 432b05ce5a99c8239fafc47a6b65d46a0fbac26e src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35331/diff/ Testing --- make check Thanks, Cong Wang
Re: Review Request 34361: converted hard-coded strings to consts
On June 10, 2015, 1:25 a.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 7:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 7:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 35341: Added Uber to the Powered-by-Mesos page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35341/#review87518 --- Bad patch! Reviews applied: [35341] Failed command: ./support/apply-review.sh -n -r 35341 Error: 2015-06-11 03:15:26 URL:https://reviews.apache.org/r/35341/diff/raw/ [832/832] - 35341.patch [1] 35341.patch:17: new blank line at EOF. + warning: 1 line adds whitespace errors. Successfully applied: Added Uber to the Powered-by-Mesos page Added Uber to the Powered-by-Mesos page Review: https://reviews.apache.org/r/35341 docs/powered-by-mesos.md:91: new blank line at EOF. Failed to commit patch - Mesos ReviewBot On June 11, 2015, 2:41 a.m., Marco Massenzio wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35341/ --- (Updated June 11, 2015, 2:41 a.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- Added Uber to the Powered-by-Mesos page Diffs - docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815 Diff: https://reviews.apache.org/r/35341/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 34910: Added task validation for task using revocable resources while its executor does not.
On June 3, 2015, 6:08 p.m., Ben Mahler wrote: src/tests/master_validation_tests.cpp, lines 1136-1137 https://reviews.apache.org/r/34910/diff/1/?file=975982#file975982line1136 I find it nice to reduce jaggedness on comments, e.g.: ``` // Create a task that uses revocable resources // while it's executor does not. ``` Vinod Kone wrote: I think most of our code just wraps the comments at 70. That is true, as that is the only hard rule we have (although it's not getting enforced everywhere :)). But subjectively, let's try to write readable comments that are less jagged, in the same spirit as we do with our source code wrapping! Maybe this means I'm signing up for adding to the style guide ;) - Ben --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/#review86449 --- On June 10, 2015, 11:56 p.m., Vinod Kone wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34910/ --- (Updated June 10, 2015, 11:56 p.m.) Review request for mesos, Ben Mahler, Ian Downes, Jie Yu, and Joris Van Remoortere. Bugs: MESOS-2753 https://issues.apache.org/jira/browse/MESOS-2753 Repository: mesos Description --- Enforces the invariant that a task cannot use revocable resources unless its executor does. Diffs - src/master/validation.hpp a74e844b39595deb9d907cc8ab1b8aee622a8765 src/master/validation.cpp 6c9dc040a7966774a1156fc260126a0f0561af28 src/slave/slave.cpp b9fb929684ae582b9133126fefa5a06d4181d836 src/tests/master_validation_tests.cpp dc9e91e120c2af9e72013557730f6a2fbb5b00fe Diff: https://reviews.apache.org/r/34910/diff/ Testing --- make check Thanks, Vinod Kone
Re: Review Request 34687: (De)Serializing MasterInfo PB to JSON
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34687/#review87520
---
How about adding an 'Address' message, which can contain 'ip' and 'port' for
now?
```
message Address {
required string ip;
required uint32 port;
// Later we can add 'hostname' or 'public_hostname', etc!
}
```
In the future, we can further use this in other protobufs to have a conistent
representation (e.g. SlaveInfo only has 'hostname' and 'port' currently, no
'ip'!). That way, you can add an address to MasterInfo:
```
message MasterInfo {
required string id = 1;
required uint32 ip = 2;
required uint32 port = 3 [default = 5050];
optional string pid = 4;
optional string hostname = 5;
optional Address address = 6;
}
```
This way, you don't need all the custom logic introduced here, and consequently
compatibility is easier to test (i.e. we have already tested our JSON -
Protobuf conversion facilities).
Have you guys considered this approach?
- Ben Mahler
On June 5, 2015, 8:18 p.m., Marco Massenzio wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34687/
---
(Updated June 5, 2015, 8:18 p.m.)
Review request for mesos, haosdent huang and Niklas Nielsen.
Bugs: MESOS-2807
https://issues.apache.org/jira/browse/MESOS-2807
Repository: mesos
Description
---
Jira: MESOS-2340
This is a preliminary step to enabling JSON API
for Master Discovery via Zookeeper.
We currently save the MasterInfo PB to ZK
serializing directly the binary data, so that
for clients to retrieve that information, they
need to either link up with libmesos or
obtain the PB definition (in mesos/mesos.proto).
This change only provides the (de)serialization
utility methods and associated tests.
Diffs
-
src/Makefile.am f2110a360b6e2a1e99e1d9630d5dfacfd818530a
src/common/http.hpp afce7fea334c7bfa57efc48c413bf906a2ebde32
src/common/http.cpp 2ac7fba7a3aac913540f1b09768777393b79284a
src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098
src/tests/common/http_tests.cpp f087b2313a13c3199b70b3d7feb728e1449a52e7
src/tests/common/parse_tests.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/34687/diff/
Testing
---
make check
Thanks,
Marco Massenzio
Re: Review Request 35333: Small fix in updateSlave() to make resource math clearer.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35333/#review87509 --- Patch looks great! Reviews applied: [35333] All tests passed. - Mesos ReviewBot On June 11, 2015, 12:07 a.m., Jiang Yan Xu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35333/ --- (Updated June 11, 2015, 12:07 a.m.) Review request for mesos and Ben Mahler. Repository: mesos Description --- A follow-up on BenM's suggestion in /r/35118/. Diffs - src/master/master.cpp 95ca2e5f6c903dea7528e559f86639e78ec92b9b Diff: https://reviews.apache.org/r/35333/diff/ Testing --- make check. Thanks, Jiang Yan Xu
Re: Review Request 34361: converted hard-coded strings to consts
On June 9, 2015, 6:25 p.m., Ben Mahler wrote: src/tests/master_tests.cpp, lines 3031-3034 https://reviews.apache.org/r/34361/diff/3/?file=971359#file971359line3031 Why bother with all this? Why not just have `key1`, `value1`, `key2`, `value2` inlined appropriately throughout these tests. Very straightforward to read. Colin Williams wrote: I think the issue with the changes remaining is that the test depends on the same value occurring in several places. By consolidating to a variable it's no longer possible for these values to get out of sync. Niklas Nielsen wrote: Colin: exactly Ben: We have label tests three places; in the master, slave and in the modules and they use the same foo, bar, baz, qux key value pairs. The idea was to centralize them, so they don't get out of date and to avoid code duplication. Does that make sense? Ben Mahler wrote: Well, then let's just keep it simple and get rid of these special strings by just making the tests use key1, value1, key2, value2, etc. I'm not following your code duplication point, this introduces more code (now there are additional global constants, which we like to avoid if unnecessary). Also, each test is ideally independent, why does the master's test for labels affect the slave's test for labels? Let's say I need a test with 5 labels, you want to have 5x2=10 global constants to address this? Try to see how testLabelKey and testLabelValue was used previously and foo, bar, qux was used on others and I created this ticket to unify the way we do this, along with seeing these key value pair being created in the slave and master tests. Dispite the current patch, I do think we can simply and unify the test label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious which key pairs are being tested. The comments in the test code need to carry a bunch of context, to make sense out of the label transformations (especially across the library border for the hooks tests). This is all in spirit of reducing the tech debt we introduced. We are on the same page on not introducing more lines/key pairs than before. Let us iterate on this and get back to you. - Niklas --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/#review87331 --- On June 8, 2015, 12:05 p.m., Colin Williams wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34361/ --- (Updated June 8, 2015, 12:05 p.m.) Review request for mesos and Niklas Nielsen. Bugs: MESOS-2637 https://issues.apache.org/jira/browse/MESOS-2637 Repository: mesos Description --- converted hard-coded strings to consts Diffs - src/tests/hook_tests.cpp 3ffde6d src/tests/master_tests.cpp ba3858f src/tests/slave_tests.cpp acae497 Diff: https://reviews.apache.org/r/34361/diff/ Testing --- Thanks, Colin Williams
Re: Review Request 35320: Fixed a bug in the fixed resource estimator.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35320/#review87511 --- Ship it! Ship It! - Niklas Nielsen On June 10, 2015, 12:48 p.m., Jie Yu wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35320/ --- (Updated June 10, 2015, 12:48 p.m.) Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone. Bugs: MESOS-2791 https://issues.apache.org/jira/browse/MESOS-2791 Repository: mesos Description --- Fixed a bug in the fixed resource estimator. Diffs - src/slave/resource_estimators/fixed.cpp 3efa18d7e3c6ac62e67f75ea45a832f3f6349036 Diff: https://reviews.apache.org/r/35320/diff/ Testing --- make check Thanks, Jie Yu
Re: Review Request 35341: Added Uber to the Powered-by-Mesos page
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35341/ --- (Updated June 10, 2015, 7:41 p.m.) Review request for mesos and Niklas Nielsen. Repository: mesos Description --- Added Uber to the Powered-by-Mesos page Diffs - docs/powered-by-mesos.md 0ee763ffe32e6667e4ba708dd275d9f1b5116815 Diff: https://reviews.apache.org/r/35341/diff/ Testing --- Thanks, Marco Massenzio
Re: Review Request 35332: Add a test case for egress flow ID recovery
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35332/#review87516 --- Patch looks great! Reviews applied: [35331, 35332] All tests passed. - Mesos ReviewBot On June 11, 2015, 1:30 a.m., Cong Wang wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35332/ --- (Updated June 11, 2015, 1:30 a.m.) Review request for mesos, Chi Zhang, Ian Downes, and Jie Yu. Bugs: MESOS-2799 https://issues.apache.org/jira/browse/MESOS-2799 Repository: mesos Description --- Add a test case for egress flow ID recovery Diffs - src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 Diff: https://reviews.apache.org/r/35332/diff/ Testing --- make check Thanks, Cong Wang
