-----------------------------------------------------------
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’t 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
>
>
