----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/31016/#review78193 -----------------------------------------------------------
Looks good. Just some minor cleanup suggestions. include/mesos/hook.hpp <https://reviews.apache.org/r/31016/#comment126704> s/overwrites/overwrite/ src/hook/manager.cpp <https://reviews.apache.org/r/31016/#comment126707> I'd prefer to avoid the `Labels*` and just substitute `taskInfo_.mutable_labels()` or `taskInfo_.labels()` at the two uses. src/hook/manager.cpp <https://reviews.apache.org/r/31016/#comment126708> What would result.isNone() mean? Please add a comment, if not a LOG(INFO). src/slave/slave.cpp <https://reviews.apache.org/r/31016/#comment126706> +1 to introducing this as high up in the method as possible, to reduce risk of using the wrong taskInfo in future nearby calls. And another +1 to renaming this `task` to prevent accidental use of the const parameter (rename it `task_`) instead of the actively modified taskInfo. - 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/31016/ > ----------------------------------------------------------- > > (Updated March 13, 2015, 4:04 p.m.) > > > Review request for mesos, Ben Mahler and Kapil Arya. > > > Repository: mesos > > > Description > ------- > > Added decorator which gets invoked on start of runTask() sequence in the > slave. > > > Diffs > ----- > > include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d > src/hook/manager.hpp da813492108974a7e26b366845368517589da876 > src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f > src/slave/slave.hpp 989832f8783d07d0702b30f0a68b8c383b57c621 > src/slave/slave.cpp 0f99e4efb8fa2b96f120a3e49191158ca0364c06 > > Diff: https://reviews.apache.org/r/31016/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Niklas Nielsen > >
