> 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.
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? - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44424/#review122377 ----------------------------------------------------------- On April 4, 2016, 1:58 a.m., Qian Zhang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44424/ > ----------------------------------------------------------- > > (Updated April 4, 2016, 1:58 a.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 > >
