> On March 8, 2016, 8: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. > > Anand Mazumdar wrote: > 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.
Agree. > 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? 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 :-) - Qian ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44424/#review122377 ----------------------------------------------------------- On April 3, 2016, 8: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, 8: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 > >
