----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/41760/#review118242 -----------------------------------------------------------
I am little concerned about `Flags` being passed to the module. Since there is no visibility about the allowed master/agent flags from the module's perspective, how does it cope with the changes to master/slave flags? Would we want to expose available master/slave flags as well in include/mesos? It's not quite clear what the correct representation would be. include/mesos/module/anonymous.hpp (line 59) <https://reviews.apache.org/r/41760/#comment179468> I am wondering if we should allow the module to indicate an error during initialization. Should the return type be `Try<Nothing>` instead? src/examples/test_anonymous_module.hpp (lines 25 - 48) <https://reviews.apache.org/r/41760/#comment179462> Let's move this to the cpp file as with the other modules. src/examples/test_anonymous_module.cpp (lines 54 - 58) <https://reviews.apache.org/r/41760/#comment179463> Do we also need a destructor just like in TestAnonymous? src/examples/test_anonymous_module.cpp (line 82) <https://reviews.apache.org/r/41760/#comment179464> Update description? src/slave/main.cpp (lines 275 - 278) <https://reviews.apache.org/r/41760/#comment179465> Just add a CHECK here and remove the if-else. The module manager returns `Error` for NULL. src/tests/anonymous_tests.cpp (lines 95 - 102) <https://reviews.apache.org/r/41760/#comment179466> Please do an explicit initialization: Try<Anonymous*> module = ModuleManager::create<Anonymous>("org_apache_mesos_TestIni..."); src/tests/anonymous_tests.cpp (line 106) <https://reviews.apache.org/r/41760/#comment179467> Is there a way to validate this typecast? src/tests/anonymous_tests.cpp (lines 106 - 107) <https://reviews.apache.org/r/41760/#comment179469> Can we just modify the module to fail during initialization if the required flags are not present and not do this check here? - Kapil Arya On Jan. 4, 2016, 3:16 p.m., Marco Massenzio wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/41760/ > ----------------------------------------------------------- > > (Updated Jan. 4, 2016, 3:16 p.m.) > > > Review request for mesos, Anand Mazumdar and Kapil Arya. > > > Bugs: MESOS-4253 > https://issues.apache.org/jira/browse/MESOS-4253 > > > Repository: mesos > > > Description > ------- > > Jira: [MESOS-4253](https://issues.apache.org/jira/browse/MESOS-4253) > > To provide a basic "context" to Anonymous modules, we pass in the `BaseFlags`. > > This tries to be backward compatible with existing modules, so a no-op > (`virtual`) implementation of the method is provided in the base class. > Currently the code is only implemented in the Agent's `main()` method, but if > deemed correct, adding it to Master is trivial. > > > Diffs > ----- > > include/mesos/module/anonymous.hpp 629eb123b9d5fa9e07ddb1c704ee72e96e8fec5b > src/examples/test_anonymous_module.hpp > 3da33a42f04b7421cee8fbb85e29b66e352f5894 > src/examples/test_anonymous_module.cpp > dd291cff3b5d47337e371cd2c1082fd6716af3fc > src/slave/main.cpp 9d48a0823189ea6505073a2803f02d90dc382ab4 > src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 > src/tests/module.hpp 8e92774ddd51bc8a1368fb1cf6546300696b2d22 > src/tests/module.cpp 7968519996ca9f9d8895e73d5f173d26a7e794e0 > > Diff: https://reviews.apache.org/r/41760/diff/ > > > Testing > ------- > > Unit tests pass. > > Also implemented in the > [execute-module](http://github.com/massenz/execute-module) - and it works > there too: > ``` > I0102 17:38:26.180788 19971 main.cpp:272] Initializing anonymous module > 'com_alertavert_mesos_RemoteExecution' > I0102 17:38:26.180800 19971 main.cpp:278] Sending flags to module > 'com_alertavert_mesos_RemoteExecution' > I0102 17:38:26.180810 19971 execute_module.hpp:129] Executing initialize() > for module; parsing runtime flags > I0102 17:38:26.181658 19971 execute_module.hpp:139] Configured work dir to > [/tmp/agent] and Sandbox dir to [/mnt/mesos/sandbox] > ``` > > > Thanks, > > Marco Massenzio > >
