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

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?


> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 120
> > <https://reviews.apache.org/r/44424/diff/2/?file=1282127#file1282127line120>
> >
> >     Let's scope all the functions after this to the `protected` namespace. 
> >     
> >     I know that you had an initial look into the example code that has them 
> > in the `public` namespace. But, most of them are generally meant to be used 
> > as simple walkthrough code-samples.
> 
> Qian Zhang wrote:
>     Can you please elaborate why making those methods (`connected()`, 
> `doReliableRegistration()`, `disconnected()`, `received()`, etc.) protected? 
> I see command executor have the similar methods (`registered()`, 
> `reregistered()`, `disconnected()`, `launchTask()`, etc.) as public too.

The previous `Executor` interface had these methods as pure virtual and one was 
required to implement these methods. Hence, the command executor implemented 
these methods in the `public` scope.

Here, in the new interface we need only the 3 methods `connected(), 
disconnected(), received()` i.e. the callback methods to be in the public scope 
and the rest of the method(s) like `launch() kill()` etc. can be part of the 
`protected` namespace.


> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 930-931
> > <https://reviews.apache.org/r/44424/diff/2/?file=1282127#file1282127line930>
> >
> >     s/unAckedUpdates/updates
> >     s/unAckedTask/task
> >     
> >     Did you think naming it as `task` might result in ambiguity?
> 
> Qian Zhang wrote:
>     Yeah, there is already a local variable `task` in the code to launch 
> task: `const TaskInfo& task = event.launch().task();`, maybe we rename this 
> local variable to `_task` to avoid ambiguity?

Sounds good.


- Anand


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


On April 3, 2016, 12:52 p.m., Qian Zhang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> -----------------------------------------------------------
> 
> (Updated April 3, 2016, 12:52 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
> -----
> 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Reply via email to