Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, lines 804-809
> > 
> >
> > Lets fix this hack now that the executor receives acknowledgements for 
> > status updates.
> > 
> > The executor can terminate after it receives an ACK for a terminal 
> > status update. Can you add a TODO here and fix it in a separate review?
> 
> Qian Zhang wrote:
> Agree, will post a separate patch for it soon.
> 
> Qian Zhang wrote:
> But what if framework does not cooperate? E.g., it does not send ACK when 
> it receives a terminal status update from executor. Then the executor will 
> never terminate? I think that may not be what we want ...

Posted the patch: https://reviews.apache.org/r/46187/


- Qian


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


On April 13, 2016, 5:13 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 13, 2016, 5:13 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Vinod Kone]

> On April 12, 2016, 8:24 p.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, lines 296-297
> > 
> >
> > No need to capture taskId because you can get it from `task`?
> 
> Qian Zhang wrote:
> I think just capuring `task` is not enough, because `task` will be set to 
> `None()` when we receive `ACKNOWLEDGED` event, but after that we will still 
> need `taskId`, e.g., in `reap()`, or when we receive `KILL` or `SHUTDOWN` 
> event.

Oh I see. You can drop this then.


- Vinod


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


On April 13, 2016, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 13, 2016, 9:13 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
> -
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 13, 2016, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 13, 2016, 9:13 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
> -
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Vinod Kone]

The ack to the executor is done by the slave as soon as it checkpoints it. 

@vinodkone

> On Apr 13, 2016, at 7:17 AM, Qian Zhang  wrote:
> 
> 
> 
>>> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
>>> src/launcher/http_command_executor.cpp, lines 804-809
>>> 
>>> 
>>>Lets fix this hack now that the executor receives acknowledgements for 
>>> status updates.
>>> 
>>>The executor can terminate after it receives an ACK for a terminal 
>>> status update. Can you add a TODO here and fix it in a separate review?
>> 
>> Qian Zhang wrote:
>>Agree, will post a separate patch for it soon.
> 
> But what if framework does not cooperate? E.g., it does not send ACK when it 
> receives a terminal status update from executor. Then the executor will never 
> terminate? I think that may not be what we want ...
> 
> 
> - Qian
> 
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/#review128522
> ---
> 
> 
>> On April 13, 2016, 5:13 p.m., Qian Zhang wrote:
>> 
>> ---
>> This is an automatically generated e-mail. To reply, visit:
>> https://reviews.apache.org/r/44424/
>> ---
>> 
>> (Updated April 13, 2016, 5:13 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
> 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, lines 804-809
> > 
> >
> > Lets fix this hack now that the executor receives acknowledgements for 
> > status updates.
> > 
> > The executor can terminate after it receives an ACK for a terminal 
> > status update. Can you add a TODO here and fix it in a separate review?
> 
> Qian Zhang wrote:
> Agree, will post a separate patch for it soon.

But what if framework does not cooperate? E.g., it does not send ACK when it 
receives a terminal status update from executor. Then the executor will never 
terminate? I think that may not be what we want ...


- Qian


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


On April 13, 2016, 5:13 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 13, 2016, 5:13 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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

(Updated April 13, 2016, 5:13 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Addressed Vinod's comments.


Bugs: MESOS-3558
https://issues.apache.org/jira/browse/MESOS-3558


Repository: mesos


Description
---

Updated http_command_executor.cpp to use v1 API.


Diffs (updated)
-

  src/launcher/http_command_executor.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44424/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, lines 296-297
> > 
> >
> > No need to capture taskId because you can get it from `task`?

I think just capuring `task` is not enough, because `task` will be set to 
`None()` when we receive `ACKNOWLEDGED` event, but after that we will still 
need `taskId`, e.g., in `reap()`, or when we receive `KILL` or `SHUTDOWN` event.


> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, line 415
> > 
> >
> > Instead of using underscores, the older version, where it explicitly 
> > uses fs:: namespace, looks cleaner.

Agree.


> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, line 764
> > 
> >
> > does the compiler require this namespacing to disambiguate?

No, thanks for catching this!


> On April 13, 2016, 4:24 a.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, lines 804-809
> > 
> >
> > Lets fix this hack now that the executor receives acknowledgements for 
> > status updates.
> > 
> > The executor can terminate after it receives an ACK for a terminal 
> > status update. Can you add a TODO here and fix it in a separate review?

Agree, will post a separate patch for it soon.


- Qian


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


On April 9, 2016, 4:40 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 9, 2016, 4:40 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Vinod Kone]

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




src/launcher/http_command_executor.cpp (lines 286 - 287)


No need to capture taskId because you can get it from `task`?



src/launcher/http_command_executor.cpp (line 405)


Instead of using underscores, the older version, where it explicitly uses 
fs:: namespace, looks cleaner.



src/launcher/http_command_executor.cpp (line 702)


does the compiler require this namespacing to disambiguate?



src/launcher/http_command_executor.cpp (lines 736 - 741)


Lets fix this hack now that the executor receives acknowledgements for 
status updates.

The executor can terminate after it receives an ACK for a terminal status 
update. Can you add a TODO here and fix it in a separate review?



src/launcher/http_command_executor.cpp (line 863)


kill this in favor of `task` below.


- Vinod Kone


On April 9, 2016, 8:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 9, 2016, 8:40 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
> -
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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


Ship it!




LGTM

- Anand Mazumdar


On April 9, 2016, 8:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 9, 2016, 8:40 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
> -
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On April 8, 2016, 6:52 p.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 82-94
> > 
> >
> > Let's move this all out of the `mesos::internal` namespace. We 
> > typically have `using` declarations at the top of the cpp file.
> > 
> > Also, I synced up with Vinod offline let's put all the cpp code in 
> > `mesos::v1::internal` namespace. Then, you won't have all the naming 
> > ambiguities that made you add the using declarations in `mesos::internal` 
> > namespace.
> 
> Qian Zhang wrote:
> Thanks Anand! And I think we do not need the followings since we are 
> already in `mesos::v1` namespace.
> using mesos::v1::CommandInfo;
> using mesos::v1::FrameworkInfo;
> using mesos::v1::KillPolicy;
> using mesos::v1::TaskID;
> using mesos::v1::TaskInfo;
> using mesos::v1::TaskState;
> using mesos::v1::TaskStatus;

Thanks for cleaning them up.


- Anand


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


On April 9, 2016, 8:40 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 9, 2016, 8:40 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
> -
> 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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

(Updated April 9, 2016, 4:40 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 (updated)
-

  src/launcher/http_command_executor.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44424/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 9, 2016, 2:52 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 82-94
> > 
> >
> > Let's move this all out of the `mesos::internal` namespace. We 
> > typically have `using` declarations at the top of the cpp file.
> > 
> > Also, I synced up with Vinod offline let's put all the cpp code in 
> > `mesos::v1::internal` namespace. Then, you won't have all the naming 
> > ambiguities that made you add the using declarations in `mesos::internal` 
> > namespace.

Thanks Anand! And I think we do not need the followings since we are already in 
`mesos::v1` namespace.
using mesos::v1::CommandInfo;
using mesos::v1::FrameworkInfo;
using mesos::v1::KillPolicy;
using mesos::v1::TaskID;
using mesos::v1::TaskInfo;
using mesos::v1::TaskState;
using mesos::v1::TaskStatus;


- Qian


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


On April 7, 2016, 10:13 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 7, 2016, 10:13 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



Looks good! Just one new major comment around moving the code to the 
`mesos::v1::internal` namespace that would allow you to deal with naming 
ambiguities better. I was playing around with your patch and the following 
github gist already has fixes for all the new issues that I have raised.

https://gist.github.com/hatred/c0be071125f6715e9403f5673de1860f


src/launcher/http_command_executor.cpp (lines 80 - 92)


Let's move this all out of the `mesos::internal` namespace. We typically 
have `using` declarations at the top of the cpp file.

Also, I synced up with Vinod offline let's put all the cpp code in 
`mesos::v1::internal` namespace. Then, you won't have all the naming 
ambiguities that made you add the using declarations in `mesos::internal` 
namespace.



src/launcher/http_command_executor.cpp (line 94)


Let's remove this out to the top of the file too. Also, let's break it into 
the following using declarations.

```cpp
using process::Clock;
using process::Future;
using process::Owned;
using process::Subprocess;
using process::Timer;
```



src/launcher/http_command_executor.cpp (line 147)


Can we log the type of received event here similar to:


https://github.com/apache/mesos/blob/master/src/examples/long_lived_executor.cpp#L98



src/launcher/http_command_executor.cpp (line 184)


Can we add a log for this similar to here:


https://github.com/apache/mesos/blob/master/src/examples/long_lived_executor.cpp#L124

You would need to separate out `ERROR` in it's own `case` statement for 
this.



src/launcher/http_command_executor.cpp (line 376)


We should be able to remove the `slave::` prefix since we already include 
it.



src/launcher/http_command_executor.cpp (line 411)


Ditto



src/launcher/http_command_executor.cpp (line 798)


We have been renaming this method as `update` in the recent reviews to be 
more in sync with the `v1` API.


- Anand Mazumdar


On April 7, 2016, 2:13 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 7, 2016, 2:13 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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

(Updated April 7, 2016, 10:13 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 (updated)
-

  src/launcher/http_command_executor.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44424/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 6, 2016, 11:14 p.m., Anand Mazumdar wrote:
> > Thanks for bearing with me. Most of the new comments are from the previous 
> > version itself. My bad, should have been caught by me earlier.

Thanks Anand for all the comments, you are really rigorous and all your review 
comments are very helpful!


> On April 6, 2016, 11:14 p.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 1082
> > 
> >
> > Nit: s/mesos::v1::FrameworkID/FrameworkID
> > 
> > You already have a `using` declaration for this.

That `using` is in the scope of `mesos::slave` namespace, but this code is out 
of that scope.


> On April 6, 2016, 11:14 p.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 1083
> > 
> >
> > Ditto

Ditto.


- Qian


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


On April 5, 2016, 8: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, 8: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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[haosdent huang]

> On April 6, 2016, 5 p.m., haosdent huang wrote:
> > src/launcher/http_command_executor.cpp, line 264
> > 
> >
> > May you add a comment why need add `Second(1)` here?
> 
> Anand Mazumdar wrote:
> It's pretty self-explanatory that it's the backoff retry timeout. No need 
> for an explicit comment.

hmm, let me drop this.


- haosdent


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


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On April 6, 2016, 5 p.m., haosdent huang wrote:
> > src/launcher/http_command_executor.cpp, line 264
> > 
> >
> > May you add a comment why need add `Second(1)` here?

It's pretty self-explanatory that it's the backoff retry timeout. No need for 
an explicit comment.


- Anand


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


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[haosdent huang]

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




src/launcher/http_command_executor.cpp (line 256)


May you add a comment why need add `Second(1)` here?


- haosdent huang


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[haosdent huang]

> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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(
>   ::taskHealthUpdated,
>   ::task_id,
>   ::healthy,
>   ::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 
> 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



Thanks for bearing with me. Most of the new comments are from the previous 
version itself. My bad, should have been caught by me earlier.


src/launcher/http_command_executor.cpp (line 240)


We have typically have the `set_type()` invocation immediatly after 
defining the `Call` itself. This helps readability and is also consistent with 
the rest of the code.

What I am suggesting is something like this:

```cpp
Call call;
call.set_type(Call::SUBSCRIBE);

call.mutable_framework_id
```

Here and all the other places please.



src/launcher/http_command_executor.cpp (line 272)


Would be good to have a comment here too similar to others for consistency.

```cpp
// Capture the task.
```



src/launcher/http_command_executor.cpp (line 287)


This should fit in one line, no?



src/launcher/http_command_executor.cpp (line 296)


Should fit in one line, no?



src/launcher/http_command_executor.cpp (line 578)


s/_task/task.get()



src/launcher/http_command_executor.cpp (line 604)


s/killTask/kill



src/launcher/http_command_executor.cpp (line 628)


s/killTask/kill



src/launcher/http_command_executor.cpp (line 709)


s/killTask/kill



src/launcher/http_command_executor.cpp (line 923)


Nit: s/mesos::v1::FrameworkID/FrameworkID

You already have a `using` declaration for this.



src/launcher/http_command_executor.cpp (line 924)


Ditto


- Anand Mazumdar


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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

(Updated April 5, 2016, 8: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 (updated)
-

  src/launcher/http_command_executor.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44424/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On April 5, 2016, 7:37 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 475-477
> > 
> >
> > Why change this? What's wrong with the previous version spanning 2 
> > lines?

The reason should be that previous the whole launch task code into `received()` 
directly then they can not fit into 2 lines, now it should be OK since we have 
moved those code into a separate method.


> On April 5, 2016, 7:37 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 1157-1165
> > 
> >
> > 2 space indent here.

I see other components does the similar, e.g.:
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp#L340:L345
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L1950:L1964

So I guess here we should have 4 space indent?


- Qian


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


On April 4, 2016, 5:11 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated April 4, 2016, 5:11 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



This looks pretty good. One more round of minor cleanups and we would get there.


src/launcher/http_command_executor.cpp (line 51)


Can you reorder alphabetically?



src/launcher/http_command_executor.cpp (line 74)


Nit: Can you move this above `std::string`?



src/launcher/http_command_executor.cpp (lines 80 - 88)


Can you reorder these alphabetically?



src/launcher/http_command_executor.cpp (line 89)


Nit: Newline before



src/launcher/http_command_executor.cpp (line 158)


s/launchTask/launch

This is more in sync with what we have been using as the name for the 
launch function for the v1 API.



src/launcher/http_command_executor.cpp (line 163)


s/killTask/kill



src/launcher/http_command_executor.cpp (lines 195 - 199)


How about:

```cpp
// TODO(qianzhang): Currently, the `mesos-health-check` binary can only 
send unversioned `TaskHealthStatus` messages. This needs to be revisited as 
part of MESOS-5103.
```



src/launcher/http_command_executor.cpp (line 216)


We use 4 line indent for function arguments. Can you fix this?



src/launcher/http_command_executor.cpp (lines 217 - 218)


I would fix the corresponding command executor to not take boolean 
arguments by reference as part of  https://reviews.apache.org/r/45707/ 

Can you fix it here?



src/launcher/http_command_executor.cpp (line 227)


Indent?



src/launcher/http_command_executor.cpp (line 260)


It looks a bit weird that the other function `kill` has an argument of just 
`TaskID` and this has an argument of `Event`.

Can we modify the argument to be just `const Task& _task`



src/launcher/http_command_executor.cpp (line 289)


2 space indent for continuation statements.



src/launcher/http_command_executor.cpp (line 297)


2 space indent for continuation statements.



src/launcher/http_command_executor.cpp (line 423)


Why new line? Also, should fit in one line, no?



src/launcher/http_command_executor.cpp (lines 464 - 466)


Why change this? What's wrong with the previous version spanning 2 lines?



src/launcher/http_command_executor.cpp (line 592)


The default argument for the last argument is anyways `None()`. Why have 
this redundant bit here? Also, would fit in one line neverthless.



src/launcher/http_command_executor.cpp (line 594)


Why not do this after L273 i.e. as soon as you have verified if a task has 
not yet been launched and right before you capture the `TaskID` i.e.

```cpp
// Capture the task.
task = _task;
```

We can then directly use it instead of using `_task`. Sound reasonable?



src/launcher/http_command_executor.cpp (lines 652 - 655)


Should fit in one line, no?

Also if we have `healthy` as a default argument, we can get rid of the 
explicit `None()` too.



src/launcher/http_command_executor.cpp (line 813)


Indent should be 4 spaces.



src/launcher/http_command_executor.cpp (line 815)


hmm ... Why isn't this argument default initialized to `None()` too?



src/launcher/http_command_executor.cpp (lines 1003 - 1011)


2 space indent here.


- Anand Mazumdar


On April 4, 2016, 9:11 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:11 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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

(Updated April 4, 2016, 5:11 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 (updated)
-

  src/launcher/http_command_executor.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/44424/diff/


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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(
>   ::taskHealthUpdated,
>   ::task_id,
>   ::healthy,
>   ::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 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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(
>   ::taskHealthUpdated,
>   ::task_id,
>   ::healthy,
>   ::kill_task);
> }
> ```
> 
> ```cpp
> void taskHealthUpdated(...)
> {
>   sendStatusUpdate(evolve(taskId), evolve(state), healthy, None());
>   
>   
> }
> ```
> 
> Does it make more sense now?

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.


- 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 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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?

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(
  ::taskHealthUpdated,
  ::task_id,
  ::healthy,
  ::kill_task);
}
```

```cpp
void taskHealthUpdated(...)
{
  sendStatusUpdate(evolve(taskId), evolve(state), healthy, None());
  
  
}
```

Does it make more sense now?


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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?

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?


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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 :-)

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?


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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

---
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 (updated)
-

  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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 120
> > 
> >
> > 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
> > 
> >
> > 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 

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

> On March 8, 2016, 12:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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
> > 
> >
> > 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
> > 
> >
> > 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
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

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


Changes
---

Addressed Anand's comments.


Bugs: MESOS-3558
https://issues.apache.org/jira/browse/MESOS-3558


Repository: mesos


Description
---

Updated http_command_executor.cpp to use v1 API.


Diffs (updated)
-

  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

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 105
> > 
> >
> > Why did you move this? Let's have the ordering of member variables the 
> > same as command executor.
> > 
> > Also thanks for resolving the ambiguity name in the name of `override` 
> > variable. I would love it that you make a similar change to the command 
> > executor code too.

Posted a patch for updating command executor: 
https://reviews.apache.org/r/45623/


> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 120
> > 
> >
> > 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.

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.


> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, line 244
> > 
> >
> > New line here. I am assuming you won't need it when you move this to an 
> > helper function.

Can you please elaborate why we do not need it when I move the code to launch 
task into the helper method `launchTask()`?


> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > src/launcher/http_command_executor.cpp, lines 930-931
> > 
> >
> > s/unAckedUpdates/updates
> > s/unAckedTask/task
> > 
> > Did you think naming it as `task` might result in ambiguity?

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?


> On March 8, 2016, 8:08 a.m., Anand Mazumdar wrote:
> > include/mesos/v1/mesos.proto, lines 1796-1813
> > 
> >
> > 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?

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.


- Qian


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


On March 6, 2016, 5:08 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated March 6, 2016, 5:08 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 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Vinod Kone]

> On March 14, 2016, 10:18 p.m., Anand Mazumdar wrote:
> > Qian, any updates on this?
> 
> Qian Zhang wrote:
> Sorry Anand, I am a little busy on the implementation of CNI support in 
> Mesos, will get back to this patch soon.

Do you still have cycles to work on this? If not, I can ask someone else to 
take it on.


- Vinod


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


On March 6, 2016, 9:08 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated March 6, 2016, 9:08 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 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Qian Zhang]

> On March 15, 2016, 6:18 a.m., Anand Mazumdar wrote:
> > Qian, any updates on this?

Sorry Anand, I am a little busy on the implementation of CNI support in Mesos, 
will get back to this patch soon.


- Qian


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


On March 6, 2016, 5:08 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated March 6, 2016, 5:08 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 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



Qian, any updates on this?

- Anand Mazumdar


On March 6, 2016, 9:08 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated March 6, 2016, 9:08 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 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



Thanks for working on this. Looks good, just one major issue around handling 
`TaskHealthStatus` messages + minor suggested cleanups.

Would love to take a more detailed look after the cleanups.


include/mesos/v1/mesos.proto (lines 1796 - 1813)


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?



src/launcher/http_command_executor.cpp (line 29)


Newline before.



src/launcher/http_command_executor.cpp (line 70)


Move this before `std::vector`



src/launcher/http_command_executor.cpp (lines 72 - 76)


Can we do a sweep and eliminate all existing occurences of types that still 
use the v1 namespace in this .cpp file even after the `using` declaration e.g., 
`v1::FrameworkId` etc. 

Also, can you add more using directives like `TaskState/TaskID`?

I am assuming after this cleanup the `.cpp` file would be free of any 
`v1::` directives.



src/launcher/http_command_executor.cpp (lines 75 - 76)


Move these  before `v1::executor::Call` i.e. above L72.



src/launcher/http_command_executor.cpp (line 77)


Nit: Can we also include all the things from `process` namespace that we 
use to be consistent like `Future/Clock/Owned` etc?



src/launcher/http_command_executor.cpp (line 99)


Why did you move this? Let's have the ordering of member variables the same 
as command executor.

Also thanks for resolving the ambiguity name in the name of `override` 
variable. I would love it that you make a similar change to the command 
executor code too.



src/launcher/http_command_executor.cpp (line 109)


Let's have the ordering of member variables the same as command executor.



src/launcher/http_command_executor.cpp (line 112)


Let's use the C++11 way of doing this:

```cpp
virtual ~HttpCommandExecutor() = default;
```



src/launcher/http_command_executor.cpp (line 113)


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.



src/launcher/http_command_executor.cpp (line 130)


Nit: Newline before.



src/launcher/http_command_executor.cpp (line 158)


Can we have the default value as `None()` here?

Would avoid passing `None()` explicitly for cases where you don't have the 
`message` argument.



src/launcher/http_command_executor.cpp (line 165)


Nit: newline before.



src/launcher/http_command_executor.cpp (line 168)


Newline before.



src/launcher/http_command_executor.cpp (line 171)


Newline before.



src/launcher/http_command_executor.cpp (line 199)


Newline before. We typically have a new-line after a statement having 
multiple lines.



src/launcher/http_command_executor.cpp (line 205)


We typically follow a procedural programming style. However, we also hate 
nested code as it's hard to read if it spans multiple lines. Let's try to 
create a function called `launch` here. Also, let's create relevant helper 
functions for all the switch cases except `ACKNOWDLEGED`/`SUBSCRIBED` since 
they seem trivial with 2-3 lines of code.

Hence, the