mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Vinod Kone <vinodk...@gmail.com>
Subject Re: Review Request 56210: Reused previous task status to generate a new one in command executor.
Date Wed, 08 Feb 2017 01:08:35 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56210/#review164584
-----------------------------------------------------------




src/launcher/executor.cpp (lines 714 - 753)
<https://reviews.apache.org/r/56210/#comment236370>

    I think these helpers are bit confusing. For example, it's not clear to me when I should
use `update()` vs `bootstrappedUpdate()` when I write new code in the future that generates
a status update (say TASK_STARTING). The comment on top of `update()` seems too specific to
health checks for the generic function names that you picked here.
    
    Is `update` supposed to be used when `TaskState` changes whereas the `bootstrappedUpdate`
should be used when it doesn't? I hope not because even if TaskState changes you would want
to preserve health and check status. Right now it kinda works because TASK_RUNNING is the
only state that can have different health / check statuses, whereas TASK_KILLING or TASK_FINISHED
or TASK_FINISHED don't need that info.
    
    Alternatively, can we just merge these 2 into one helper that takes a bunch of optional
fields that can overwrite fields in `lastTaskStatus`? Would that be more intuitive?



src/launcher/executor.cpp (line 720)
<https://reviews.apache.org/r/56210/#comment236351>

    looks like this review doesn't use `reason` argument, so I wouldn't add it in this review.
lets add it in the review that needs it.



src/launcher/executor.cpp (line 752)
<https://reviews.apache.org/r/56210/#comment236352>

    ditto. no `reason` argument in this review.



src/launcher/executor.cpp (line 775)
<https://reviews.apache.org/r/56210/#comment236350>

    s/sendTaskStatusUpdate/forward/


- 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/56210/
> -----------------------------------------------------------
> 
> (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
> -------
> 
> When a new task status update is generated in the executor, we have
> to make sure specific data is duplicated from the previous update
> to, e.g., avoid shadowing of those data during reconciliation. For
> instance, consider a check status being sent; in this status update
> we must include the latest known health information.
> 
> 
> Diffs
> -----
> 
>   src/launcher/executor.cpp 0c770bb18ae8bd8df85589b5262f457ab50574a9 
> 
> Diff: https://reviews.apache.org/r/56210/diff/
> 
> 
> Testing
> -------
> 
> See https://reviews.apache.org/r/56218/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message