> 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.
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. > On Nov. 24, 2014, 10:12 p.m., Alexander Rukletsov wrote: > > src/master/main.cpp, lines 159-164 > > <https://reviews.apache.org/r/28401/diff/1/?file=774585#file774585line159> > > > > See my comment above. The message is a bit misleading: there can be a > > situation, when this error is printed, but several hooks are loaded. > > Kapil Arya wrote: > The error message is supposed to inform the user of a failed hook > loading. Is there a alternative message that you suggest? Yes, according to my comment above tuned by the modules philosophy, I would suggest to reject loading all hooks and print the list of failed hooks. However, the message should state clear that though only subset of hooks failed, none of them were loaded. > On Nov. 24, 2014, 10:12 p.m., Alexander Rukletsov wrote: > > src/hook/manager.cpp, lines 74-78 > > <https://reviews.apache.org/r/28401/diff/1/?file=774583#file774583line74> > > > > Is my understanding correct, that if we have plenty of hook modules > > loaded, we will search through all of them each time a hook is invoked > > (e.g. authorizeTasks())? If yes, then together with the hook's payload it > > can lead to performance issues. > > Kapil Arya wrote: > This is a good question. There are two parts to the performance penalty: > > 1. Iterating through all the available modules -- for this case, we can > try keeping a separate hook list for each "kind" of hook. > 2. Hook overhead -- As with the design of hooks, there is (yet) no > guarantee that a hook won't misbehave. 1. I would propose to make hooks very specific (see my comment in r28402) and keep a list of registered (available) hooks per each hookpoint. That should eliminate lookup. 2. As I mentioned on the dev list, I would propose to be able to switch hooks completely at compile time. - 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 > >
