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/02 09:57:35 UTC
Review Request 56212: Added support for general checks to command
executor.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs
-----
src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9
Diff: https://reviews.apache.org/r/56212/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Vinod Kone <vi...@gmail.com>.
> On Feb. 8, 2017, 1:32 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, line 327
> > <https://reviews.apache.org/r/56212/diff/1/?file=1622062#file1622062line327>
> >
> > can we also add a REASON_TASK_HEALTHCHECK_STATUS_UPDATED for consistency?
>
> Alexander Rukletsov wrote:
> Absolutely! We already have a JIRA for this: https://issues.apache.org/jira/browse/MESOS-6905
Can you add a TODO here for clarity?
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review164608
-----------------------------------------------------------
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/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 8, 2017, 1:32 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, line 453
> > <https://reviews.apache.org/r/56212/diff/1/?file=1622062#file1622062line453>
> >
> > there is a lot of code duplication in the call sites of checker and health checker and their code itself. I know you had a TODO, but can you ensure there is a ticket for tracking the de-duplication of code? we should tackle that sooner than latter, hopefully even before 1.3 is cut. otherwise it will be a nightmare to maintain both.
I could not agree more. https://issues.apache.org/jira/browse/MESOS-7092
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review164608
-----------------------------------------------------------
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/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 8, 2017, 1:32 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, line 327
> > <https://reviews.apache.org/r/56212/diff/1/?file=1622062#file1622062line327>
> >
> > can we also add a REASON_TASK_HEALTHCHECK_STATUS_UPDATED for consistency?
Absolutely! We already have a JIRA for this: https://issues.apache.org/jira/browse/MESOS-6905
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review164608
-----------------------------------------------------------
On Feb. 8, 2017, 3:16 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 3:16 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review164608
-----------------------------------------------------------
src/launcher/executor.cpp (line 327)
<https://reviews.apache.org/r/56212/#comment236371>
can we also add a REASON_TASK_HEALTHCHECK_STATUS_UPDATED for consistency?
src/launcher/executor.cpp (line 453)
<https://reviews.apache.org/r/56212/#comment236372>
there is a lot of code duplication in the call sites of checker and health checker and their code itself. I know you had a TODO, but can you ensure there is a ticket for tracking the de-duplication of code? we should tackle that sooner than latter, hopefully even before 1.3 is cut. otherwise it will be a nightmare to maintain both.
- Vinod Kone
On Feb. 2, 2017, 9:57 a.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 2, 2017, 9:57 a.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On Feb. 14, 2017, 6:59 p.m., Gast�n Kleiman wrote:
> > src/launcher/executor.cpp, lines 848-854
> > <https://reviews.apache.org/r/56212/diff/3/?file=1627706#file1627706line848>
> >
> > This method is called to build updates when the task transitions to `TASK_KILLING`, `TASK_FINISHED`, `TASK_KILLED`, and `TASK_FAILED`.
> >
> > Do we want to lose the result of the previous check in all those cases? I think it might make sense to keep it at least for `TASK_KILLING`. It would also make sense to keep the `health` flag in that case.
I'm not sure what is the best thing to do here. Does it make sense to talk about check status when, e.g., the task is being killed?
Moreover, for health checks, keeping health info for terminal states means changing current behaviour.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review165550
-----------------------------------------------------------
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/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review165550
-----------------------------------------------------------
src/launcher/executor.cpp (lines 848 - 854)
<https://reviews.apache.org/r/56212/#comment237416>
This method is called to build updates when the task transitions to `TASK_KILLING`, `TASK_FINISHED`, `TASK_KILLED`, and `TASK_FAILED`.
Do we want to lose the result of the previous check in all those cases? I think it might make sense to keep it at least for `TASK_KILLING`. It would also make sense to keep the `health` flag in that case.
- 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/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 8, 2017, 4:56 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
>
> Diff: https://reviews.apache.org/r/56212/diff/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56218/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated March 15, 2017, 8:49 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
Diff: https://reviews.apache.org/r/56212/diff/8/
Changes: https://reviews.apache.org/r/56212/diff/7-8/
Testing
-------
See https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated March 15, 2017, 12:51 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Changes
-------
Rebased. NNTR.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
Diff: https://reviews.apache.org/r/56212/diff/7/
Changes: https://reviews.apache.org/r/56212/diff/6-7/
Testing
-------
See https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review168977
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 14, 2017, 2:09 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated March 14, 2017, 2:09 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
>
>
> Diff: https://reviews.apache.org/r/56212/diff/6/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated March 14, 2017, 2:09 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
Diff: https://reviews.apache.org/r/56212/diff/6/
Changes: https://reviews.apache.org/r/56212/diff/5-6/
Testing
-------
See https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review168408
-----------------------------------------------------------
hmm. for some reason i don't get the publish link when i try to add my comment as a reply to the issue! probably an RB bug.
posting it here:
1) Add a TODO referring to the JIRA ticket on top of the block that sets `healthy` field.
For posterity to avoid confusing future devs that come across this.
2) Can we actually add a `protobuf::createTaskStatus` overload in protobuf utils that creates a task status from scratch
and have `protobuf::createStatusUpdate` and the `executor::createTaskStatus` call that?
3) It isn\u2019t intuitive to me why callers were setting `healthy` as an arg while calling `executor::createTaskStatus` whereas `check_status` was not being controlled by the callers? Maybe refactor so callers are responsible or add a TODO/comment.
4) I think using `unacknowledgeTask` in this helper (whether you move it to callers after the refactor) is buggy because that will be reset to `None` as soon as the first ack for the first update is received you should be storing `TaskInfo` separately irrespective of unacknowledged state maybe do a `struct TaskData { TaskInfo taskinfo; bool acknowledged;}`
- Vinod Kone
On March 7, 2017, 8:39 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated March 7, 2017, 8:39 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
>
>
> Diff: https://reviews.apache.org/r/56212/diff/5/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated March 7, 2017, 8:39 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
Diff: https://reviews.apache.org/r/56212/diff/5/
Changes: https://reviews.apache.org/r/56212/diff/4-5/
Testing
-------
See https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
> On March 3, 2017, 2:10 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp
> > Lines 880 (patched)
> > <https://reviews.apache.org/r/56212/diff/4/?file=1651753#file1651753line880>
> >
> > Can you remind me again, why this logic is different for health checks vs checks? for one, what to set into `healthy` field is passed as an argument to this function but what to set in `check_status` is calculated inside. second, whether a health check is defined or not doesn't have any bearing on the internals of this function, but check does.
Re: second, we should fix it eventually, see https://issues.apache.org/jira/browse/MESOS-6417. Re: first, it's not entirely true: another routine from `protobuf::` is used to create a task status with check status set, hence this function does not take it as an argument. What this function does is setting an "empty" task check status.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review167782
-----------------------------------------------------------
On Feb. 28, 2017, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:54 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
>
>
> Diff: https://reviews.apache.org/r/56212/diff/4/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/#review167782
-----------------------------------------------------------
src/launcher/executor.cpp
Lines 880 (patched)
<https://reviews.apache.org/r/56212/#comment239745>
Can you remind me again, why this logic is different for health checks vs checks? for one, what to set into `healthy` field is passed as an argument to this function but what to set in `check_status` is calculated inside. second, whether a health check is defined or not doesn't have any bearing on the internals of this function, but check does.
- Vinod Kone
On Feb. 28, 2017, 3:54 p.m., Alexander Rukletsov wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56212/
> -----------------------------------------------------------
>
> (Updated Feb. 28, 2017, 3:54 p.m.)
>
>
> Review request for mesos, Gast�n Kleiman and Vinod Kone.
>
>
> Bugs: MESOS-6906
> https://issues.apache.org/jira/browse/MESOS-6906
>
>
> Repository: mesos
>
>
> Description
> -------
>
> See summary.
>
>
> Diffs
> -----
>
> src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
>
>
> Diff: https://reviews.apache.org/r/56212/diff/4/
>
>
> Testing
> -------
>
> See https://reviews.apache.org/r/56213/
>
>
> Thanks,
>
> Alexander Rukletsov
>
>
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated Feb. 28, 2017, 3:54 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp adcc50fb7df91b9f8dff0f583b61e0efcd6da378
Diff: https://reviews.apache.org/r/56212/diff/
Testing (updated)
-------
See https://reviews.apache.org/r/56213/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 4:56 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
Diff: https://reviews.apache.org/r/56212/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov
Re: Review Request 56212: Added support for general checks to command
executor.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56212/
-----------------------------------------------------------
(Updated Feb. 8, 2017, 3:16 p.m.)
Review request for mesos, Gast�n Kleiman and Vinod Kone.
Bugs: MESOS-6906
https://issues.apache.org/jira/browse/MESOS-6906
Repository: mesos
Description
-------
See summary.
Diffs (updated)
-----
src/launcher/executor.cpp d9417ce1d5b108f5292a66010eab80f11552a969
Diff: https://reviews.apache.org/r/56212/diff/
Testing
-------
See https://reviews.apache.org/r/56218/
Thanks,
Alexander Rukletsov