You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Niklas Nielsen <ni...@qni.dk> on 2015/03/14 00:04:14 UTC
Re: Review Request 31016: Added slave run task decorator.
-----------------------------------------------------------
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.
Changes
-------
Rebased
Repository: mesos
Description
-------
Added decorator which gets invoked on start of runTask() sequence in the slave.
Diffs (updated)
-----
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
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:26 a.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1186-1188
> > <https://reviews.apache.org/r/31016/diff/4/?file=920381#file920381line1186>
> >
> > What makes this the ideal place to do the label decoration? Looks like this is wedged between setting up different unschedule calls. Seems like we should either decorate the labels as early as possible, right after (or before?) the pid/id/state checks; or, if we need to delay it, we could do it after the unschedules.
Great point - I will probably move it up before the unschedule code. Does that sound OK to you?
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review79803
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Adam B <ad...@mesosphere.io>.
> On April 11, 2015, 3:26 a.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1186-1188
> > <https://reviews.apache.org/r/31016/diff/4/?file=920381#file920381line1186>
> >
> > What makes this the ideal place to do the label decoration? Looks like this is wedged between setting up different unschedule calls. Seems like we should either decorate the labels as early as possible, right after (or before?) the pid/id/state checks; or, if we need to delay it, we could do it after the unschedules.
>
> Niklas Nielsen wrote:
> Great point - I will probably move it up before the unschedule code. Does that sound OK to you?
Yeah, I'm thinking earlier is better, so we don't early-exit without updating labels.
- Adam
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review79803
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:26 a.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1186-1188
> > <https://reviews.apache.org/r/31016/diff/4/?file=920381#file920381line1186>
> >
> > What makes this the ideal place to do the label decoration? Looks like this is wedged between setting up different unschedule calls. Seems like we should either decorate the labels as early as possible, right after (or before?) the pid/id/state checks; or, if we need to delay it, we could do it after the unschedules.
>
> Niklas Nielsen wrote:
> Great point - I will probably move it up before the unschedule code. Does that sound OK to you?
>
> Adam B wrote:
> Yeah, I'm thinking earlier is better, so we don't early-exit without updating labels.
>
> Niklas Nielsen wrote:
> Took that for a spin and we need the framework info, so we need to have it after the unschedule block :/
Could use the bare info - the hook is moved up in the latest patch.
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review79803
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On April 11, 2015, 3:26 a.m., Adam B wrote:
> > src/slave/slave.cpp, lines 1186-1188
> > <https://reviews.apache.org/r/31016/diff/4/?file=920381#file920381line1186>
> >
> > What makes this the ideal place to do the label decoration? Looks like this is wedged between setting up different unschedule calls. Seems like we should either decorate the labels as early as possible, right after (or before?) the pid/id/state checks; or, if we need to delay it, we could do it after the unschedules.
>
> Niklas Nielsen wrote:
> Great point - I will probably move it up before the unschedule code. Does that sound OK to you?
>
> Adam B wrote:
> Yeah, I'm thinking earlier is better, so we don't early-exit without updating labels.
Took that for a spin and we need the framework info, so we need to have it after the unschedule block :/
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review79803
-----------------------------------------------------------
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review79803
-----------------------------------------------------------
Looks good. Just a question about when we should actually do the decoration.
src/slave/slave.cpp
<https://reviews.apache.org/r/31016/#comment129386>
What makes this the ideal place to do the label decoration? Looks like this is wedged between setting up different unschedule calls. Seems like we should either decorate the labels as early as possible, right after (or before?) the pid/id/state checks; or, if we need to delay it, we could do it after the unschedules.
- Adam B
On April 11, 2015, 3:03 a.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 11, 2015, 3:03 a.m.)
>
>
> Review request for mesos, Ben Mahler and Kapil Arya.
>
>
> Bugs: MESOS-2351
> https://issues.apache.org/jira/browse/MESOS-2351
>
>
> 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review80820
-----------------------------------------------------------
Ship it!
Ship It!
- Adam B
On April 20, 2015, 1:44 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 20, 2015, 1:44 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
> -------
>
> 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 9495c704ca4bde4ab283d12efa3ea9b2f1158a4c
> src/slave/slave.cpp 8ec80ed26f338690e0a1e712065750ab77a724cd
> src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22
> src/tests/mesos.cpp fc534e9febed1e293076e00e0f5c3879a78df90f
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/
-----------------------------------------------------------
(Updated April 20, 2015, 1:44 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
-------
Added decorator which gets invoked on start of runTask() sequence in the slave.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/hook/manager.hpp da813492108974a7e26b366845368517589da876
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/slave/slave.hpp 9495c704ca4bde4ab283d12efa3ea9b2f1158a4c
src/slave/slave.cpp 8ec80ed26f338690e0a1e712065750ab77a724cd
src/tests/mesos.hpp 7744df55a2a31446327da7bd2b16457e90711d22
src/tests/mesos.cpp fc534e9febed1e293076e00e0f5c3879a78df90f
Diff: https://reviews.apache.org/r/31016/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/
-----------------------------------------------------------
(Updated April 11, 2015, 3:03 a.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Bugs: MESOS-2351
https://issues.apache.org/jira/browse/MESOS-2351
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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
Diff: https://reviews.apache.org/r/31016/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/
-----------------------------------------------------------
(Updated April 7, 2015, 5:57 p.m.)
Review request for mesos, Ben Mahler and Kapil Arya.
Changes
-------
Addressed Adam's and Kapil's comments.
Repository: mesos
Description
-------
Added decorator which gets invoked on start of runTask() sequence in the slave.
Diffs (updated)
-----
include/mesos/hook.hpp 9ae8b9455a86c7a5cbf4f1d1b1ce88f2811ce35d
src/hook/manager.hpp da813492108974a7e26b366845368517589da876
src/hook/manager.cpp 7a4cb09bc221af502e867cfb7fff2900b599ff1f
src/slave/slave.hpp 19e6b44bc344c0ca509674803f401cbb4e1f47ae
src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
Diff: https://reviews.apache.org/r/31016/diff/
Testing
-------
make check
Thanks,
Niklas Nielsen
Re: Review Request 31016: Added slave run task decorator.
Posted by Niklas Nielsen <ni...@qni.dk>.
> On March 30, 2015, 3:44 a.m., Adam B wrote:
> > src/slave/slave.cpp, line 1173
> > <https://reviews.apache.org/r/31016/diff/3/?file=894746#file894746line1173>
> >
> > +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.
There has been a lot of discussion around exactly this pattern (unconsting, making a copy and what to call things). The cleanest way seems to be to make the parameter non-const (make the compiler make a copy) and set the labels inline. That way we avoid the helper too.
Did this in the last patch - please take a look :)
- Niklas
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/31016/#review78193
-----------------------------------------------------------
On April 7, 2015, 5:57 p.m., Niklas Nielsen wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31016/
> -----------------------------------------------------------
>
> (Updated April 7, 2015, 5:57 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 19e6b44bc344c0ca509674803f401cbb4e1f47ae
> src/slave/slave.cpp 9fec023b643d410f4d511fa6f80e9835bab95b7e
>
> Diff: https://reviews.apache.org/r/31016/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Niklas Nielsen
>
>
Re: Review Request 31016: Added slave run task decorator.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
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
>
>