You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rukletsov <ru...@gmail.com> on 2017/02/08 15:20:31 UTC
Review Request 56449: Moved health checker closer to container in
default executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs
-----
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
Diff: https://reviews.apache.org/r/56449/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review167861
-----------------------------------------------------------
No updates to this diff yet?
- Vinod Kone
On Feb. 28, 2017, 2:35 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 2:35 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
>
>
> Diff: https://reviews.apache.org/r/56449/diff/2/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review169265
-----------------------------------------------------------
src/launcher/default_executor.cpp
Line 873 (original)
<https://reviews.apache.org/r/56449/#comment241610>
LOG or VLOG here?
src/launcher/default_executor.cpp
Lines 876 (patched)
<https://reviews.apache.org/r/56449/#comment241611>
what's the guarantee that the `healthChecker` is `Some` here? AFAICT `kill()` sets this to None() without erasing the container from the map.
- Vinod Kone
On March 16, 2017, 4:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:48 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
>
>
> Diff: https://reviews.apache.org/r/56449/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated March 23, 2017, 12:10 p.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Changes
-------
Rebased. NNTR.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs (updated)
-----
src/launcher/default_executor.cpp 6a885af50db6d489194cc9b780fcf1494e853694
Diff: https://reviews.apache.org/r/56449/diff/5/
Changes: https://reviews.apache.org/r/56449/diff/4-5/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review169448
-----------------------------------------------------------
Ship it!
Ship It!
- Gast�n Kleiman
On March 20, 2017, 11:54 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated March 20, 2017, 11:54 a.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
>
>
> Diff: https://reviews.apache.org/r/56449/diff/4/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated March 20, 2017, 11:54 a.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs (updated)
-----
src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
Diff: https://reviews.apache.org/r/56449/diff/4/
Changes: https://reviews.apache.org/r/56449/diff/3-4/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review169281
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 16, 2017, 4:48 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated March 16, 2017, 4:48 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Bugs: MESOS-7249
> https://issues.apache.org/jira/browse/MESOS-7249
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
>
>
> Diff: https://reviews.apache.org/r/56449/diff/3/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated March 16, 2017, 4:48 p.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs
-----
src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
Diff: https://reviews.apache.org/r/56449/diff/3/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated March 16, 2017, 4:41 p.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Bugs: MESOS-7249
https://issues.apache.org/jira/browse/MESOS-7249
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs (updated)
-----
src/launcher/default_executor.cpp cbd4f7ecd042e7fa603bd69774d95472df2c896d
Diff: https://reviews.apache.org/r/56449/diff/3/
Changes: https://reviews.apache.org/r/56449/diff/2-3/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review169155
-----------------------------------------------------------
Patch looks great!
Reviews applied: [56016, 57394, 56017, 57594, 56208, 57149, 56210, 56211, 57596, 57150, 56212, 57597, 57598, 56213, 56449]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
On Feb. 28, 2017, 2:35 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 2:35 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
>
>
> Diff: https://reviews.apache.org/r/56449/diff/2/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 2:35 p.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs
-----
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
Diff: https://reviews.apache.org/r/56449/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 15, 2017, 10 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp
> > Lines 869 (patched)
> > <https://reviews.apache.org/r/56449/diff/2/?file=1627710#file1627710line875>
> >
> > What's the guarantee that this task is still present in `containers`? AFAICT this is a deferred callback invoked by the health checker.
You're right: we erase the taskId right after we reap the task container, while a check can be in flight and report its status afterwards.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review165770
-----------------------------------------------------------
On Feb. 28, 2017, 2:35 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 2:35 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
>
>
> Diff: https://reviews.apache.org/r/56449/diff/2/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review165770
-----------------------------------------------------------
src/launcher/default_executor.cpp (lines 635 - 637)
<https://reviews.apache.org/r/56449/#comment237631>
Now that this TODO is copied to `kill()`, don't think it makes sense to have it here?
src/launcher/default_executor.cpp (line 869)
<https://reviews.apache.org/r/56449/#comment237630>
What's the guarantee that this task is still present in `containers`? AFAICT this is a deferred callback invoked by the health checker.
- Vinod Kone
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
>
> Diff: https://reviews.apache.org/r/56449/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/#review165695
-----------------------------------------------------------
Ship it!
src/launcher/default_executor.cpp (line 639)
<https://reviews.apache.org/r/56449/#comment237555>
Looks like making `healthChecker` an `Option` doesn't gain us much, since we still have to be defensive and do `CHECK_NOTNULL` before using it.
- Gast�n Kleiman
On Feb. 8, 2017, 4:56 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56449/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> With the recent introduction of the `Container` struct in the default
> executor, tasks' health checkers should be moved to this struct. Also,
> health checking is stopped on per-task basis and not on shutdown.
>
>
> Diffs
> -----
>
> src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
>
> Diff: https://reviews.apache.org/r/56449/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56449: Moved health checker closer to container in
default executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56449/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 4:56 p.m.)
Review request for mesos, Anand Mazumdar, Gast�n Kleiman, and Vinod Kone.
Repository: mesos
Description
-------
With the recent introduction of the `Container` struct in the default
executor, tasks' health checkers should be moved to this struct. Also,
health checking is stopped on per-task basis and not on shutdown.
Diffs (updated)
-----
src/launcher/default_executor.cpp e63cf153831088851863d0956455a024e9bc172a
Diff: https://reviews.apache.org/r/56449/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov