> On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > src/examples/example_module_impl.cpp, line 34 > > <https://reviews.apache.org/r/27051/diff/1/?file=728879#file728879line34> > > > > Why not do this in the constructor?
We want to catch the failure and return a NULL pointer. If we do it in constructor, there is no good way to return an error message. > On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > src/examples/example_module_impl.cpp, lines 83-84 > > <https://reviews.apache.org/r/27051/diff/1/?file=728879#file728879line83> > > > > Won't this be cleaner if you do 'initialize' in the constructor? Same reasoning as above. We want to capture error and return NULL on failure. > On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > src/module/manager.cpp, line 233 > > <https://reviews.apache.org/r/27051/diff/1/?file=728886#file728886line233> > > > > Where do you guard for has_parameters()? No need to check for it. If no parameters were specified, parameters() will return a default empty object of type Parameters. > On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > include/mesos/module.hpp, line 49 > > <https://reviews.apache.org/r/27051/diff/1/?file=728878#file728878line49> > > > > 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. Create a new JIRA ticket: https://issues.apache.org/jira/browse/MESOS-1981 > On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > src/slave/flags.hpp, lines 354-355 > > <https://reviews.apache.org/r/27051/diff/1/?file=728887#file728887line354> > > > > Same question as above Fixed. > On Oct. 23, 2014, 5:06 p.m., Niklas Nielsen wrote: > > src/tests/flags.hpp, lines 101-102 > > <https://reviews.apache.org/r/27051/diff/1/?file=728888#file728888line101> > > > > Same question as above Fixed. - Kapil ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/27051/#review58083 ----------------------------------------------------------- On Oct. 23, 2014, 5:55 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, 5:55 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 > >
