mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 56210: Reused previous task status to generate a new one in command executor.
Date Thu, 09 Feb 2017 11:53:02 GMT


> 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?

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?


- Alexander


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


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/56210/
> -----------------------------------------------------------
> 
> (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
> -------
> 
> 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