> On Nov. 24, 2014, 10:12 p.m., Alexander Rukletsov wrote: > > src/hook/manager.cpp, lines 51-62 > > <https://reviews.apache.org/r/28401/diff/1/?file=774583#file774583line51> > > > > This code loads hooks one by one and gives up once the next hook cannot > > be found, right? This looks a bit strange for me. I would propose one of > > the two alternatives: > > 1. Optimistic: load all loadable hooks and ignore those that cannot be > > found. > > 2. Pessimistic, but transactional: load hooks only if all given hooks > > can be found & loaded. > > Kapil Arya wrote: > This is in alignment with the module design philosophy. If something that > was specified on the command line is not correct/valid, we should fail there > instead of going forward with partial success. > > So, if a single hook fails to load, we fail write there. The user can > then either fix the problem or remove the hook from the command-line flag. > > Alexander Rukletsov wrote: > But shouldn't we than rollback all loaded hooks? Otherwise, if there is a > hook that fails, then the order of the hooks on the commandline will yield > different behaviour. > > Kapil Arya wrote: > If loading a hook fails, we eventually end up doing an exit(), so not > unloading them doesn't have any side-effects (unless I am missing something > here). > > About the order of the hooks on the commandline, yes, the error message > will be different if a different hook (that has now been placed in the front > of the list) fails. But I think this is okay. We are informing the user of > one problem at a time. Alternatively, we could try loading all hooks and > then present the user with a list of all hooks that failed to load. > Currently, we are following the design from mesos modules, but if you feel > strongly about it, we should discuss changing modulemanager as well.
So we do an `exit()` if a hook fails? Can be ok for the first version, but this seems too much for me. We may want adding and removing hooks on the fly, without stopping the process (this will be possible with https://issues.apache.org/jira/browse/MESOS-2090). Though we do an `exit()` now, it's not visible here and this behaviour may change. I would go for commit-or-rollback semantics here. Maybe we should also do it for modules, though hooks feel for me "more optional" than modules. > On Nov. 24, 2014, 10:12 p.m., Alexander Rukletsov wrote: > > src/hook/hook.hpp, line 37 > > <https://reviews.apache.org/r/28401/diff/1/?file=774581#file774581line37> > > > > I'm not sure we can use tuples. > > Kapil Arya wrote: > I am not sure if I follow. Do you mean we can't use them because of > C++11 or more generally? Sorry for being unclear. Yes, C++11. If we start using them, let's check all officially supported compilers (i.e. gcc 4.4) handle them and if so, add them to the list of allowed c++11 features. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28401/#review62879 ----------------------------------------------------------- On Nov. 24, 2014, 8:07 p.m., Kapil Arya wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28401/ > ----------------------------------------------------------- > > (Updated Nov. 24, 2014, 8:07 p.m.) > > > Review request for mesos, Benjamin Hindman and Niklas Nielsen. > > > Bugs: MESOS-2060 > https://issues.apache.org/jira/browse/MESOS-2060 > > > Repository: mesos-git > > > Description > ------- > > Hooks can be specified on the command line by specifying the --hooks flags. > Hooks are implemented as Mesos modules that have to be specified via the > --modules flag which can also be used to specify any hook-specific command > line parameters. > > This is still a PoC based on the discussions on the dev mailing list. > > > Diffs > ----- > > src/Makefile.am 1d4ba1c8335eb8106cbccf903eaf3d9fdebdcda2 > src/hook/hook.hpp PRE-CREATION > src/hook/manager.hpp PRE-CREATION > src/hook/manager.cpp PRE-CREATION > src/master/flags.hpp 1cea50c02f3ad7de1e1ae91d65d1accdb9af7b03 > src/master/main.cpp 193d53f13d8b14638b311cc290b5a5802ea56299 > src/master/master.cpp de42f8eb7c3c4ed64fb7fea9f4977e276f4a9043 > src/module/hook.hpp PRE-CREATION > src/module/manager.cpp b15b0fc3f056fe29bd4d1acca508d75805ef2e0b > > Diff: https://reviews.apache.org/r/28401/diff/ > > > Testing > ------- > > > Thanks, > > Kapil Arya > >
