You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Benjamin Hindman <be...@berkeley.edu> on 2012/03/20 01:55:15 UTC
Review Request: Additional logic needed for CommandInfo (see description).
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4408/
-----------------------------------------------------------
Review request for mesos, John Sirois and Vinod Kone.
Summary
-------
Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave)."[commandinfo_exit 4858fe1] Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave).
Diffs
-----
src/common/try.hpp ed5611b
src/launcher/executor.cpp aac1053
src/master/master.cpp 0222bb2
src/messages/messages.proto 11a2c41
src/slave/reaper.cpp 93e47e7
src/slave/slave.hpp de304cb
src/slave/slave.cpp ddd1874
Diff: https://reviews.apache.org/r/4408/diff
Testing
-------
make check
Thanks,
Benjamin
Re: Review Request: Additional logic needed for CommandInfo (see
description).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On 2012-03-20 15:45:01, John Sirois wrote:
> >
Thank you, submitting.
> On 2012-03-20 15:45:01, John Sirois wrote:
> > src/common/try.hpp, line 41
> > <https://reviews.apache.org/r/4408/diff/1/?file=93749#file93749line41>
> >
> > why not private / force Try::error("") ?
Because I figured the code looked better when you do something like:
Try<T> t;
if (conditional) {
t = Try<T>::some(expression);
} else {
t = Try<T>::some(expression);
}
if (t.isSome()) {
...
than:
Try<T> t = Try<T>::error("Uninitialized Try");
...
> On 2012-03-20 15:45:01, John Sirois wrote:
> > src/master/master.cpp, line 1440
> > <https://reviews.apache.org/r/4408/diff/1/?file=93751#file93751line1440>
> >
> > Are there sanity checks anywhere that task.has_command() != task.has_executor()
Great question! Yes, when you try and launch a task we make sure that only one of CommandInfo or ExecutorInfo is set (and fail the task if not).
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4408/#review6122
-----------------------------------------------------------
On 2012-03-20 00:55:15, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4408/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 00:55:15)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Summary
> -------
>
> Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave)."[commandinfo_exit 4858fe1] Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave).
>
>
> Diffs
> -----
>
> src/common/try.hpp ed5611b
> src/launcher/executor.cpp aac1053
> src/master/master.cpp 0222bb2
> src/messages/messages.proto 11a2c41
> src/slave/reaper.cpp 93e47e7
> src/slave/slave.hpp de304cb
> src/slave/slave.cpp ddd1874
>
> Diff: https://reviews.apache.org/r/4408/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin
>
>
Re: Review Request: Additional logic needed for CommandInfo (see
description).
Posted by John Sirois <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4408/#review6122
-----------------------------------------------------------
Ship it!
src/common/try.hpp
<https://reviews.apache.org/r/4408/#comment13154>
why not private / force Try::error("") ?
src/master/master.cpp
<https://reviews.apache.org/r/4408/#comment13152>
Are there sanity checks anywhere that task.has_command() != task.has_executor()
- John
On 2012-03-20 00:55:15, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4408/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 00:55:15)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Summary
> -------
>
> Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave)."[commandinfo_exit 4858fe1] Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave).
>
>
> Diffs
> -----
>
> src/common/try.hpp ed5611b
> src/launcher/executor.cpp aac1053
> src/master/master.cpp 0222bb2
> src/messages/messages.proto 11a2c41
> src/slave/reaper.cpp 93e47e7
> src/slave/slave.hpp de304cb
> src/slave/slave.cpp ddd1874
>
> Diff: https://reviews.apache.org/r/4408/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin
>
>
Re: Review Request: Additional logic needed for CommandInfo (see
description).
Posted by Benjamin Hindman <be...@berkeley.edu>.
> On 2012-03-20 22:10:29, Vinod Kone wrote:
> > src/launcher/executor.cpp, line 66
> > <https://reviews.apache.org/r/4408/diff/1/?file=93750#file93750line66>
> >
> > redundant due to the CHECK above
Fixed now in https://reviews.apache.org/r/4420.
> On 2012-03-20 22:10:29, Vinod Kone wrote:
> > src/slave/reaper.cpp, line 62
> > <https://reviews.apache.org/r/4408/diff/1/?file=93753#file93753line62>
> >
> > why do you need this?
You could imagine a user attaching to a running executor to debug. This stops the process, but it might be resumed. We don't want to treat the stop as an exited/terminated executor.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4408/#review6106
-----------------------------------------------------------
On 2012-03-20 00:55:15, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4408/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 00:55:15)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Summary
> -------
>
> Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave)."[commandinfo_exit 4858fe1] Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave).
>
>
> Diffs
> -----
>
> src/common/try.hpp ed5611b
> src/launcher/executor.cpp aac1053
> src/master/master.cpp 0222bb2
> src/messages/messages.proto 11a2c41
> src/slave/reaper.cpp 93e47e7
> src/slave/slave.hpp de304cb
> src/slave/slave.cpp ddd1874
>
> Diff: https://reviews.apache.org/r/4408/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin
>
>
Re: Review Request: Additional logic needed for CommandInfo (see
description).
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/4408/#review6106
-----------------------------------------------------------
Ship it!
i somehow failed to hit publish on it yesterday..sorry!
src/launcher/executor.cpp
<https://reviews.apache.org/r/4408/#comment13134>
redundant due to the CHECK above
src/slave/reaper.cpp
<https://reviews.apache.org/r/4408/#comment13135>
why do you need this?
- Vinod
On 2012-03-20 00:55:15, Benjamin Hindman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/4408/
> -----------------------------------------------------------
>
> (Updated 2012-03-20 00:55:15)
>
>
> Review request for mesos, John Sirois and Vinod Kone.
>
>
> Summary
> -------
>
> Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave)."[commandinfo_exit 4858fe1] Made sure the slave sends status updates for tasks launched via a CommandInfo and updated Task to make 'executor_id' optional for the cases when a task was launched with a CommandInfo (w/ corresponding changes in the master and slave).
>
>
> Diffs
> -----
>
> src/common/try.hpp ed5611b
> src/launcher/executor.cpp aac1053
> src/master/master.cpp 0222bb2
> src/messages/messages.proto 11a2c41
> src/slave/reaper.cpp 93e47e7
> src/slave/slave.hpp de304cb
> src/slave/slave.cpp ddd1874
>
> Diff: https://reviews.apache.org/r/4408/diff
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Benjamin
>
>