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 Fri, 10 Feb 2017 01:32:08 GMT


> On Feb. 8, 2017, 1:08 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, line 720
> > <https://reviews.apache.org/r/56210/diff/1/?file=1622060#file1622060line720>
> >
> >     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.
> 
> Alexander Rukletsov wrote:
>     A'd rather keep it here, because I would like API to look consistent. What are general
things people should probably set if they generate a status update? State, reason, message.
They are not specific to any particular update or feature (e.g., check update).

"I would like API to look consistent" : Consistent with what? It's not clear to me why someone
creating a status update only cares about state/reason/message. That seems very specific to
this use case. If I have the executor sending a TASK_STARTING update for example, I cannot
use either of these helpers as they exist in this review.


> On Feb. 8, 2017, 1:08 a.m., Vinod Kone wrote:
> > src/launcher/executor.cpp, lines 714-753
> > <https://reviews.apache.org/r/56210/diff/1/?file=1622060#file1622060line714>
> >
> >     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?
> 
> Alexander Rukletsov wrote:
>     I think there is value in having two helpers: one for creating an update from scratch
and one for creating an update from a previous one. We can merge them, but I'd prefer having
them separate because they clear express the intent.
>     
>     Now, it's hard to say when to use one and when the other. It's up to the framework's
writer whether they want to preserve certain data or not. I doubt we should tell people to
use the bootstrapped version by default, since it implies sending more data over the network.
I think currently we should leave this decision to an executor writer and come up with a guideline
in the nearest future. I have a related RR here: https://reviews.apache.org/r/56017/ and will
plan to start a thread on the devlist about it.
>     
>     What we should do though, is to explain what these two helpers do so that executor
authors can use them wisely. Does this sound like a good plan to you?

I agree that having 2 different helpers might be useful, but the current signatures (names
and args) of these helpers are not intuitive to me. If you want to tackle the problem of right
helper function in r/56017, I would rather you avoid creating this helper in this review and
just inline the code with a TODO. I really want to avoid introducing non-intuitive helper
functions.


- Vinod


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


On Feb. 9, 2017, 12:56 p.m., Alexander Rukletsov wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56210/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 12: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
> -------
> 
> 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 d9417ce1d5b108f5292a66010eab80f11552a969 
> 
> 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