----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30961/#review78189 -----------------------------------------------------------
Minor cleanup/suggestions, but otherwise good. We'll definitely need to document te module(-manager) API change of overriding the label set instead of merging. This should probably go in the upgrades doc? src/examples/test_hook_module.cpp <https://reviews.apache.org/r/30961/#comment126700> Did you mix up key and Value? Would think you'd want the MESOS_Test_Remove_Label to be the key. src/hook/manager.cpp <https://reviews.apache.org/r/30961/#comment126702> Why not just return `taskInfo_.labels()`? In fact, I don't think there's really any reason to have a `labels` pointer in this function. Just do `taskInfo_.mutable_labels()->CopyFrom(result.get());` and then it's clear throughout that you're modifying the same `taskInfo_` object. src/tests/hook_tests.cpp <https://reviews.apache.org/r/30961/#comment126701> Switch Key, Value - Adam B On March 13, 2015, 4:04 p.m., Niklas Nielsen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/30961/ > ----------------------------------------------------------- > > (Updated March 13, 2015, 4:04 p.m.) > > > Review request for mesos, Ben Mahler and Kapil Arya. > > > Bugs: MESOS-2351 > https://issues.apache.org/jira/browse/MESOS-2351 > > > Repository: mesos > > > Description > ------- > > See summary. > > > Diffs > ----- > > include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d > src/examples/test_hook_module.cpp 47409cd4d02e238d1d182571d92019114662cd41 > src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f > src/master/master.cpp dccd7c635da4b7031cd109bd84e7f17b31777ef1 > src/tests/hook_tests.cpp bb9de25bd2c4601d333a3ca1aec13820c7df7378 > > Diff: https://reviews.apache.org/r/30961/diff/ > > > Testing > ------- > > make check (with modified VerifyMasterLaunchTaskHook test) > > > Thanks, > > Niklas Nielsen > >
