You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2017/08/09 13:14:36 UTC
Review Request 61524: Moved task validation from `getExecutorInfo` to
`runTask` on agent.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61524/
-----------------------------------------------------------
Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
Bugs: MESOS-7871
https://issues.apache.org/jira/browse/MESOS-7871
Repository: mesos
Description
-------
Previously, `getExecutorInfo` was called only in `runTask`, so it was
validating an invariant related to launching a single task. The
invariant is that a task should have either CommandInfo or ExecutorInfo
set but not both. Now `getExecutorInfo` is also called to calculate
allocated resources which might be related to a task group, which might
violate the invariant.
Diffs
-----
src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b
Diff: https://reviews.apache.org/r/61524/diff/1/
Testing
-------
sudo make check
Thanks,
Andrei Budnik
Re: Review Request 61524: Moved task validation from
`getExecutorInfo` to `runTask` on agent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 9, 2017, 6:04 p.m., Benjamin Mahler wrote:
> > src/slave/slave.cpp
> > Lines 4987-4989 (original), 4987-4989 (patched)
> > <https://reviews.apache.org/r/61524/diff/1/?file=1793980#file1793980line4991>
> >
> > Perhaps a note here saying that the master injects the executor for tasks within a task group?
In the interest of time, I followed up with patches to add a new test and update comments:
https://reviews.apache.org/r/61534/
https://reviews.apache.org/r/61535/
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61524/#review182502
-----------------------------------------------------------
On Aug. 9, 2017, 1:14 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61524/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2017, 1:14 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7871
> https://issues.apache.org/jira/browse/MESOS-7871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `getExecutorInfo` was called only in `runTask`, so it was
> validating an invariant related to launching a single task. The
> invariant is that a task should have either CommandInfo or ExecutorInfo
> set but not both. Now `getExecutorInfo` is also called to calculate
> allocated resources which might be related to a task group, which might
> violate the invariant.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b
>
>
> Diff: https://reviews.apache.org/r/61524/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61524: Moved task validation from
`getExecutorInfo` to `runTask` on agent.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Aug. 9, 2017, 6:04 p.m., Benjamin Mahler wrote:
> > src/slave/slave.cpp
> > Lines 4987-4989 (original), 4987-4989 (patched)
> > <https://reviews.apache.org/r/61524/diff/1/?file=1793980#file1793980line4991>
> >
> > Perhaps a note here saying that the master injects the executor for tasks within a task group?
>
> Greg Mann wrote:
> In the interest of time, I followed up with patches to add a new test and update comments:
> https://reviews.apache.org/r/61534/
> https://reviews.apache.org/r/61535/
I included the comment updates from https://reviews.apache.org/r/61535/ in this patch when committing.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61524/#review182502
-----------------------------------------------------------
On Aug. 9, 2017, 1:14 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61524/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2017, 1:14 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7871
> https://issues.apache.org/jira/browse/MESOS-7871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `getExecutorInfo` was called only in `runTask`, so it was
> validating an invariant related to launching a single task. The
> invariant is that a task should have either CommandInfo or ExecutorInfo
> set but not both. Now `getExecutorInfo` is also called to calculate
> allocated resources which might be related to a task group, which might
> violate the invariant.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b
>
>
> Diff: https://reviews.apache.org/r/61524/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61524: Moved task validation from
`getExecutorInfo` to `runTask` on agent.
Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61524/#review182502
-----------------------------------------------------------
Ship it!
Can we also add a test that ensures that getting the metrics succeeds when task groups were launched?
src/slave/slave.cpp
Lines 4987-4989 (original), 4987-4989 (patched)
<https://reviews.apache.org/r/61524/#comment258417>
Perhaps a note here saying that the master injects the executor for tasks within a task group?
- Benjamin Mahler
On Aug. 9, 2017, 1:14 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61524/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2017, 1:14 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7871
> https://issues.apache.org/jira/browse/MESOS-7871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `getExecutorInfo` was called only in `runTask`, so it was
> validating an invariant related to launching a single task. The
> invariant is that a task should have either CommandInfo or ExecutorInfo
> set but not both. Now `getExecutorInfo` is also called to calculate
> allocated resources which might be related to a task group, which might
> violate the invariant.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b
>
>
> Diff: https://reviews.apache.org/r/61524/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 61524: Moved task validation from
`getExecutorInfo` to `runTask` on agent.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/61524/#review182510
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Aug. 9, 2017, 1:14 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61524/
> -----------------------------------------------------------
>
> (Updated Aug. 9, 2017, 1:14 p.m.)
>
>
> Review request for mesos, Benjamin Mahler, Greg Mann, and Vinod Kone.
>
>
> Bugs: MESOS-7871
> https://issues.apache.org/jira/browse/MESOS-7871
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Previously, `getExecutorInfo` was called only in `runTask`, so it was
> validating an invariant related to launching a single task. The
> invariant is that a task should have either CommandInfo or ExecutorInfo
> set but not both. Now `getExecutorInfo` is also called to calculate
> allocated resources which might be related to a task group, which might
> violate the invariant.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b
>
>
> Diff: https://reviews.apache.org/r/61524/diff/1/
>
>
> Testing
> -------
>
> sudo make check
>
>
> Thanks,
>
> Andrei Budnik
>
>