> On March 8, 2016, 8: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?
OK, let's get the high priority tasks done first, I will file a JIRA and add a TODO. - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44424/#review122377 ----------------------------------------------------------- On April 4, 2016, 9: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, 9: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 > >
