mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From haosdent huang <haosd...@gmail.com>
Subject Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.
Date Wed, 06 Apr 2016 16:48:37 GMT


> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > <https://reviews.apache.org/r/44424/diff/2/?file=1282126#file1282126line1796>
> >
> >     hmmm .. Did you test if the health check workflow works?
> >     
> >     IIUC, the `mesos-health-check` binary sends a `TaskHealthStatus` message back
to the executor and that message is not of type `v1::TaskHealthStatus`. If we try to deserialize,
it should fail at that point. 
> >     
> >     For now, it seems to me that the best course of action is to preserve/keep using
the unversioned health check binary/message. In future, we might want to either modify the
existing `mesos-health-check` binary to emit `v1::TaskHealthStatus` messages in addition to
the unversioned ones or create a new binary for versioned health checks. I would recommend
filing a JIRA and a TODO in the code mentioning this. Makes sense?
> 
> Qian Zhang wrote:
>     Thanks for the comment! I think `TaskHealthStatus` and `v1:: TaskHealthStatus` have
exactly same fields, so it should be OK to do serialize/deserialize between them, right? Actually
all the Call messages sent by this HTTP command executor are v1, and agent is always trying
to receive non-v1 messages, I see there is no issues between them.
> 
> Anand Mazumdar wrote:
>     Looks like there is some confusion here.
>     
>     Regarding your comment:
>     "Actually all the Call messages sent by this HTTP command executor are v1, and agent
is always trying to receive non-v1 messages"
>     
>     This is _not_ how it works. The executor sends the `v1` protobuf and the agent devolves
them to an unversioned one before passing it on to the internal code. https://github.com/apache/mesos/blob/master/src/slave/http.cpp#L242
>     
>     Also, I would be _really_ surprised if protobuf's allow you to mix and match between
different messages if the fields are the same. The descriptors for both the messages are still
not the same.
>     
>     Does my original issue make more sense now?
> 
> Qian Zhang wrote:
>     Yeah, I agree with you!
>     > For now, it seems to me that the best course of action is to preserve/keep using
the unversioned health check binary/message.
>     
>     I am afraid that we can not keep using the unversioned one in this HTTP command executor,
the reason is, in the unversioned `TaskHealthStatus`, the field `task_id` is of type "mesos::TaskID"
rather than "mesos::v1::TaskID", but the rest of the this HTTP command executor codes use
"mesos::v1::TaskID", so there will be some compilation errors if we use the unversioned one,
like:
>     `error: no viable conversion from 'const mesos::TaskID' to 'const mesos::v1::TaskID'`
>     
>     Maybe now we should modify the existing `mesos-health-check` by introducing a new
string flag (e.g., `--executor_version`), its default value is `unversioned`, but this HTTP
command executor will set its value to `v1`, so when `mesos-health-check` is launched, it
will know which `TaskHealthStatus` message should be sent. Please let me know your comment
:-)
> 
> Anand Mazumdar wrote:
>     Why can't you use the `evolve` function to convert the `mesos::TaskID` received from
`mesos-health-check` to `mesos::v1::TaskID` or am I missing something?
> 
> Qian Zhang wrote:
>     I am not sure if I get your point. Did you mean we call the `evolve` function to
do the conversion in HTTP command executor? Can you please elaborate where we can call it
in the code?
> 
> Anand Mazumdar wrote:
>     Sorry, by bad. I should have included a code snippet.
>     
>     ```cpp
>     virtual void initialize()
>     {
>       // A big TODO about why we are still handling the unversioned protobuf message
with a possible link to the JIRA.
>       install<TaskHealthStatus>(
>               &CommandExecutorProcess::taskHealthUpdated,
>               &TaskHealthStatus::task_id,
>               &TaskHealthStatus::healthy,
>               &TaskHealthStatus::kill_task);
>     }
>     ```
>     
>     ```cpp
>     void taskHealthUpdated(...)
>     {
>       sendStatusUpdate(evolve(taskId), evolve(state), healthy, None());
>       
>       ....
>     }
>     ```
>     
>     Does it make more sense now?
> 
> Qian Zhang wrote:
>     Yes, it does make sense now. But I think it is kind of temp solution, why not we
modify `mesos-health-check` now to make it can send `v1:: TaskHealthStatus` message as I suggested
above (I can file a JIRA and post a patch for it)? And then for this patch, we can use `v1::
TaskHealthStatus` so that all the protobuf messages in this HTTP command executor are v1.
> 
> Anand Mazumdar wrote:
>     FWIW, I don't quite know how we would like to tackle this in the future. The solution
proposed by you is one possible route. Another possible solution can be to create a separate
health check binary itself among others? Hence, I was proposing to tackle this as part of
a separate JIRA issue later and not worry about it for now.
>     
>     There are still plenty of things that need to be done for this change itself e.g.,
tests for the HTTP command executor + getting the implementation right. I would like to worry
about those first and keep taking little steps. Sounds reasonable?
> 
> Qian Zhang wrote:
>     OK, let's get the high priority tasks done first, I will file a JIRA and add a TODO.

I think the reason why use unversioned message is we could add utils to convert form v1 to
unversioned. In the future, if we have release v2, we could continue to add utils to convert
from v2 to unversioned. And for internal communication, the protocol should be unversioned
which could change in every minor release while the v1 should be stable after Mesos release
1.0.0.


- haosdent


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


On April 5, 2016, 12:36 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> -----------------------------------------------------------
> 
> (Updated April 5, 2016, 12:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
>     https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Updated http_command_executor.cpp to use v1 API.
> 
> 
> Diffs
> -----
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>


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