----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27051/#review58083 -----------------------------------------------------------
include/mesos/module.hpp <https://reviews.apache.org/r/27051/#comment98974> Let's start the api versioning jira/doc. We need to codify the way we do version bumping. Something that describes the new flags/parameter argument. Btw - the test module version cannot be bumped individually :-/ just an observation, but we need to be careful with component/kind api changes while having a release in flight. src/examples/example_module_impl.cpp <https://reviews.apache.org/r/27051/#comment98977> Why not do this in the constructor? src/examples/example_module_impl.cpp <https://reviews.apache.org/r/27051/#comment98986> I had to think twice and look at how the test is used to get your use of the parameters here. Can you add a comment of the expected use in context of the test? src/examples/example_module_impl.cpp <https://reviews.apache.org/r/27051/#comment98988> Won't this be cleaner if you do 'initialize' in the constructor? src/examples/test_module.hpp <https://reviews.apache.org/r/27051/#comment98989> Why this include here? If you are not using it in the header, how about just including it in the cpp file? src/examples/test_module.hpp <https://reviews.apache.org/r/27051/#comment98996> See question in test file - how about extending the constructor instead? src/master/flags.hpp <https://reviews.apache.org/r/27051/#comment98993> Can we find a bit more realistic/saying key value pair? :-) src/module/isolator.hpp <https://reviews.apache.org/r/27051/#comment98995> Where is string used? In the cpp file? If so, let's move it there. src/module/isolator.hpp <https://reviews.apache.org/r/27051/#comment98994> Again, where is this used? In the .cpp file? src/module/manager.cpp <https://reviews.apache.org/r/27051/#comment98997> Where do you guard for has_parameters()? src/slave/flags.hpp <https://reviews.apache.org/r/27051/#comment98998> Same question as above src/tests/flags.hpp <https://reviews.apache.org/r/27051/#comment98999> Same question as above src/tests/module_tests.cpp <https://reviews.apache.org/r/27051/#comment99000> key/value seems a bit too generic here to understand the context - how about parameterKey, parameterValue? - Niklas Nielsen On Oct. 23, 2014, 1:47 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/27051/ > ----------------------------------------------------------- > > (Updated Oct. 23, 2014, 1:47 p.m.) > > > Review request for mesos, Benjamin Hindman, Niklas Nielsen, and Till > Toenshoff. > > > Bugs: MESOS-1896 > https://issues.apache.org/jira/browse/MESOS-1896 > > > Repository: mesos-git > > > Description > ------- > > The Modules protobuf is enhance to support key-value parameters based on > mesos::Parameter and mesos::Parameters. > > Bumped Module API version since we modified the signature of create() method. > > > Diffs > ----- > > include/mesos/module.hpp 5bafb402162d0f2c86b8802937b96ba518cea226 > src/examples/example_module_impl.cpp > 6a48d670d8df15b6da3e7e5d91be5cc2973bd0ee > src/examples/test_isolator_module.cpp > 2d6b427e1adba8df958c35e7468ef74446b3ea07 > src/examples/test_module.hpp 820df234ee4471eb354a3985dba2f31514744690 > src/master/flags.hpp 9d068564e52dbd2c98a3689c204e56e097ed0e6f > src/messages/messages.proto 6e49fe7c91a1e171a45764fe0432c20f5f14d133 > src/module/isolator.hpp fc78b072e0ce2c54e350bebd0419e9af5ae0e57e > src/module/manager.hpp dc789218db05b29ff93db5284797f1daf0028e90 > src/module/manager.cpp 63056a4813d74d8a2bdb0233e477842e627d940f > src/slave/flags.hpp 03c62a2fd040768392c7e24d93f64ca3a855c4a1 > src/tests/flags.hpp 189fad9a8125aa8f76a7abadc330a7f0ec7cc337 > src/tests/module.cpp 45becade73c79b15e737e325f73185715a01eeca > src/tests/module_tests.cpp 45125d86bbc76b7ed2b2f630fff878a0d548a0e7 > > Diff: https://reviews.apache.org/r/27051/diff/ > > > Testing > ------- > > Added new tests and ran make check. > > > Thanks, > > Kapil Arya > >
