mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Niklas Nielsen" <...@qni.dk>
Subject Re: Review Request 31016: Added slave run task decorator.
Date Tue, 24 Mar 2015 00:50:12 GMT


> On Feb. 22, 2015, 4:48 p.m., Kapil Arya wrote:
> > src/slave/slave.cpp, line 1111
> > <https://reviews.apache.org/r/31016/diff/1/?file=863935#file863935line1111>
> >
> >     Can we instead swap its name with `task_` and move the label decorator higher
(right after entry into this function)?
> >     
> >     This would prevent future developers from accidentally using `task` when they
meant to use `task_`.
> >     
> >     Doing the label decoration before the error checks has an added benefit of generating
notifications for dropped tasks as well for the hook modules.

I don't think we should reveal task infos that are failing. How should the module implementer
know the difference between a successfully launched task and a failed one?

The pattern is to append '_' as f(), f'(), f''(). So would keep that for consistency.


> On Feb. 22, 2015, 4:48 p.m., Kapil Arya wrote:
> > src/slave/slave.cpp, line 3926
> > <https://reviews.apache.org/r/31016/diff/1/?file=863935#file863935line3926>
> >
> >     const ref?

Great catch! Thanks


> On Feb. 22, 2015, 4:48 p.m., Kapil Arya wrote:
> > src/slave/slave.cpp, lines 3924-3934
> > <https://reviews.apache.org/r/31016/diff/1/?file=863935#file863935line3924>
> >
> >     Should we rather move it close to the runTask definition? Since the two are
logically close.

The ordering is slightly inconsistent (should follow the one from the header file).

I'd prefer not to cludder the code and methods around runTask() with helpers - there is a
lot of central pieces there already. Are you feeling strongly about moving this up?


- Niklas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review73457
-----------------------------------------------------------


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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message