You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Benjamin Bannier <bb...@apache.org> on 2019/06/20 21:45:19 UTC
Re: Review Request 70907: Added recovery of agent drain information.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/
-----------------------------------------------------------
(Updated June 20, 2019, 11:45 p.m.)
Review request for mesos, Greg Mann and Joseph Wu.
Changes
-------
Address Greg's comments
Summary (updated)
-----------------
Added recovery of agent drain information.
Bugs: MESOS-9822
https://issues.apache.org/jira/browse/MESOS-9822
Repository: mesos
Description
-------
With this patch the agent will, after executor reregistration finished,
replay any active drain information so remaining tasks are drained as
well. We need to wait until executors had a chance to register so they
are not terminated should we try to send kill task request before the
executor has registered.
Diffs (updated)
-----
src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
src/slave/state.cpp b4bf6292eb482f4e99a27442af0ea03e31c1ddc2
src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
Diff: https://reviews.apache.org/r/70907/diff/2/
Changes: https://reviews.apache.org/r/70907/diff/1-2/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216040
-----------------------------------------------------------
Patch looks great!
Reviews applied: [70822, 70839, 70834, 70835, 70836, 70899, 70900, 70901, 70903, 70904, 70912, 70906, 70907]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose --disable-libtool-wrappers --disable-parallel-test-execution' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On June 20, 2019, 9:45 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 9:45 p.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp b4bf6292eb482f4e99a27442af0ea03e31c1ddc2
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70907/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216035
-----------------------------------------------------------
Bad review!
Reviews applied: [70907, 70906, 70904, 70903, 70901, 70900, 70899, 70836, 70835, 70834, 70839, 70822]
Error:
2019-06-21 03:36:28 URL:https://reviews.apache.org/r/70907/diff/raw/ [6759/6759] -> "70907.patch" [1]
error: patch failed: src/slave/state.cpp:203
error: src/slave/state.cpp: patch does not apply
- Mesos Reviewbot
On June 20, 2019, 9:45 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 9:45 p.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp b4bf6292eb482f4e99a27442af0ea03e31c1ddc2
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70907/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Benjamin Bannier <bb...@apache.org>.
> On June 27, 2019, 9:23 a.m., Greg Mann wrote:
> > src/slave/slave.cpp
> > Line 5635 (original), 5638 (patched)
> > <https://reviews.apache.org/r/70907/diff/2-5/?file=2151492#file2151492line5638>
> >
> > Hmm is it really OK to `std::move()` the drain config here?
It is currently okay since `Slave::drain` does not read the its `Slave::drainConfig`, but instead overwrites it immediately with the received value. That being said, I agree that this introduces coupling which might be hard to evolve safely. Removed the `move`.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216178
-----------------------------------------------------------
On June 28, 2019, 11:27 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 28, 2019, 11:27 a.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
> src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
>
>
> Diff: https://reviews.apache.org/r/70907/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216178
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/slave.cpp
Line 5635 (original), 5638 (patched)
<https://reviews.apache.org/r/70907/#comment303240>
Hmm is it really OK to `std::move()` the drain config here?
- Greg Mann
On June 26, 2019, 11:55 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 26, 2019, 11:55 p.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
> src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
>
>
> Diff: https://reviews.apache.org/r/70907/diff/5/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216569
-----------------------------------------------------------
Ship it!
Ship It!
- Joseph Wu
On June 28, 2019, 2:27 a.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 28, 2019, 2:27 a.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
> src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
>
>
> Diff: https://reviews.apache.org/r/70907/diff/7/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/
-----------------------------------------------------------
(Updated June 28, 2019, 11:27 a.m.)
Review request for mesos, Greg Mann and Joseph Wu.
Changes
-------
Fix formatting
Bugs: MESOS-9822
https://issues.apache.org/jira/browse/MESOS-9822
Repository: mesos
Description
-------
With this patch the agent will, after executor reregistration finished,
replay any active drain information so remaining tasks are drained as
well. We need to wait until executors had a chance to register so they
are not terminated should we try to send kill task request before the
executor has registered.
Diffs (updated)
-----
src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
Diff: https://reviews.apache.org/r/70907/diff/7/
Changes: https://reviews.apache.org/r/70907/diff/6-7/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/
-----------------------------------------------------------
(Updated June 27, 2019, 1:55 a.m.)
Review request for mesos, Greg Mann and Joseph Wu.
Changes
-------
Rename a variable missed in previous rebase
Bugs: MESOS-9822
https://issues.apache.org/jira/browse/MESOS-9822
Repository: mesos
Description
-------
With this patch the agent will, after executor reregistration finished,
replay any active drain information so remaining tasks are drained as
well. We need to wait until executors had a chance to register so they
are not terminated should we try to send kill task request before the
executor has registered.
Diffs (updated)
-----
src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
Diff: https://reviews.apache.org/r/70907/diff/5/
Changes: https://reviews.apache.org/r/70907/diff/4-5/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/
-----------------------------------------------------------
(Updated June 26, 2019, 2:35 p.m.)
Review request for mesos, Greg Mann and Joseph Wu.
Changes
-------
Rebase
Bugs: MESOS-9822
https://issues.apache.org/jira/browse/MESOS-9822
Repository: mesos
Description
-------
With this patch the agent will, after executor reregistration finished,
replay any active drain information so remaining tasks are drained as
well. We need to wait until executors had a chance to register so they
are not terminated should we try to send kill task request before the
executor has registered.
Diffs (updated)
-----
src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
src/slave/state.cpp e0a850e03fb0d726bf7b02bbd6b298d81afae399
src/tests/slave_tests.cpp 7ba2df9fef9966cd09e0af3f4ef3bb260b6167f6
Diff: https://reviews.apache.org/r/70907/diff/4/
Changes: https://reviews.apache.org/r/70907/diff/3-4/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Benjamin Bannier <bb...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/
-----------------------------------------------------------
(Updated June 24, 2019, 8:08 a.m.)
Review request for mesos, Greg Mann and Joseph Wu.
Changes
-------
Use stored path instead of recomputing
Bugs: MESOS-9822
https://issues.apache.org/jira/browse/MESOS-9822
Repository: mesos
Description
-------
With this patch the agent will, after executor reregistration finished,
replay any active drain information so remaining tasks are drained as
well. We need to wait until executors had a chance to register so they
are not terminated should we try to send kill task request before the
executor has registered.
Diffs (updated)
-----
src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
src/slave/state.cpp b4bf6292eb482f4e99a27442af0ea03e31c1ddc2
src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
Diff: https://reviews.apache.org/r/70907/diff/3/
Changes: https://reviews.apache.org/r/70907/diff/2-3/
Testing
-------
`make check`
Thanks,
Benjamin Bannier
Re: Review Request 70907: Added recovery of agent drain information.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/70907/#review216022
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/state.cpp
Lines 210 (patched)
<https://reviews.apache.org/r/70907/#comment302975>
Make use of the local variable `agentStatePath` here.
- Greg Mann
On June 20, 2019, 9:45 p.m., Benjamin Bannier wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/70907/
> -----------------------------------------------------------
>
> (Updated June 20, 2019, 9:45 p.m.)
>
>
> Review request for mesos, Greg Mann and Joseph Wu.
>
>
> Bugs: MESOS-9822
> https://issues.apache.org/jira/browse/MESOS-9822
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With this patch the agent will, after executor reregistration finished,
> replay any active drain information so remaining tasks are drained as
> well. We need to wait until executors had a chance to register so they
> are not terminated should we try to send kill task request before the
> executor has registered.
>
>
> Diffs
> -----
>
> src/slave/slave.cpp 30039b0857a4d85b4b96fa95d7f8724d57cdec6e
> src/slave/state.hpp 45836e5805a5038c0b3b5563ea995c3fa90ab808
> src/slave/state.cpp b4bf6292eb482f4e99a27442af0ea03e31c1ddc2
> src/tests/slave_tests.cpp c2035976713abb31b3646c0d23771fa40df93271
>
>
> Diff: https://reviews.apache.org/r/70907/diff/2/
>
>
> Testing
> -------
>
> `make check`
>
>
> Thanks,
>
> Benjamin Bannier
>
>