Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Vinod Kone]

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

Ship it!



src/slave/slave.cpp (line 3039)


backticks for `statusUpdate()` ?


- Vinod Kone


On Nov. 21, 2015, 1:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40563/
> ---
> 
> (Updated Nov. 21, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3476
> https://issues.apache.org/jira/browse/MESOS-3476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the ability for the agent to handle status updates from HTTP 
> based executors. Previously, the existing `statusUpdate` method used to 
> handle status updates sent from slave with `pid == UPID()`, sent from other 
> executors/on behalf of other executors with `pid == Some()`. This change 
> modifies the argument to be `Option`. This ensures that `pid == None()` 
> when set, the ACK is correctly routed to the corresponding HTTP based 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
>   src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
>   src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 
> 
> Diff: https://reviews.apache.org/r/40563/diff/
> 
> 
> Testing
> ---
> 
> make check + would add tests once the https://reviews.apache.org/r/39297/ 
> chain is reviewed.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Anand Mazumdar]

> On Nov. 23, 2015, 11:38 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 3040
> > 
> >
> > backticks for `statusUpdate()` ?

There are multiple occurences in the present file that don't use backticks for 
statusUpdate. Hence, did not do it.

https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1311
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L1835
https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3080


- Anand


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


On Nov. 21, 2015, 1:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40563/
> ---
> 
> (Updated Nov. 21, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3476
> https://issues.apache.org/jira/browse/MESOS-3476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the ability for the agent to handle status updates from HTTP 
> based executors. Previously, the existing `statusUpdate` method used to 
> handle status updates sent from slave with `pid == UPID()`, sent from other 
> executors/on behalf of other executors with `pid == Some()`. This change 
> modifies the argument to be `Option`. This ensures that `pid == None()` 
> when set, the ACK is correctly routed to the corresponding HTTP based 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
>   src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
>   src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 
> 
> Diff: https://reviews.apache.org/r/40563/diff/
> 
> 
> Testing
> ---
> 
> make check + would add tests once the https://reviews.apache.org/r/39297/ 
> chain is reviewed.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [40560, 40561, 40562, 40570, 40563]

Passed command: export OS=ubuntu:14.04;export CONFIGURATION="--verbose";export 
COMPILER=gcc; ./support/docker_build.sh

- Mesos ReviewBot


On Nov. 21, 2015, 1:45 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40563/
> ---
> 
> (Updated Nov. 21, 2015, 1:45 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3476
> https://issues.apache.org/jira/browse/MESOS-3476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the ability for the agent to handle status updates from HTTP 
> based executors. Previously, the existing `statusUpdate` method used to 
> handle status updates sent from slave with `pid == UPID()`, sent from other 
> executors/on behalf of other executors with `pid == Some()`. This change 
> modifies the argument to be `Option`. This ensures that `pid == None()` 
> when set, the ACK is correctly routed to the corresponding HTTP based 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
>   src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
>   src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 
> 
> Diff: https://reviews.apache.org/r/40563/diff/
> 
> 
> Testing
> ---
> 
> make check + would add tests once the https://reviews.apache.org/r/39297/ 
> chain is reviewed.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Anand Mazumdar]

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

Review request for mesos, Ben Mahler, Isabel Jimenez, and Vinod Kone.


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


Repository: mesos


Description
---

This change adds the ability for the agent to handle status updates from HTTP 
based executors. Previously, the existing `statusUpdate` method used to handle 
status updates sent from slave with `pid == UPID()`, sent from other 
executors/on behalf of other executors with `pid == Some()`. This change 
modifies the argument to be `Option`. This ensures that `pid == None()` 
when set, the ACK is correctly routed to the corresponding HTTP based executor.


Diffs
-

  src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
  src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
  src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 

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


Testing
---

make check + would add tests once the https://reviews.apache.org/r/39297/ chain 
is reviewed.


Thanks,

Anand Mazumdar

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Anand Mazumdar]

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

(Updated Nov. 21, 2015, 12:21 a.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
Vinod Kone.


Changes
---

Modified deps


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


Repository: mesos


Description
---

This change adds the ability for the agent to handle status updates from HTTP 
based executors. Previously, the existing `statusUpdate` method used to handle 
status updates sent from slave with `pid == UPID()`, sent from other 
executors/on behalf of other executors with `pid == Some()`. This change 
modifies the argument to be `Option`. This ensures that `pid == None()` 
when set, the ACK is correctly routed to the corresponding HTTP based executor.


Diffs
-

  src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
  src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
  src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 

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


Testing
---

make check + would add tests once the https://reviews.apache.org/r/39297/ chain 
is reviewed.


Thanks,

Anand Mazumdar

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Vinod Kone]

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



src/slave/http.cpp (lines 318 - 322)


indent by 4 spaces.

also, it's hard to tell that None() is the argument for statusUpdate() and 
not createStatusUpdate()



src/slave/slave.cpp (line 2764)


this will be printed by itself on a new line.

just do;

LOG(INFO) << "Handling status update " << update
  << (pid.isSome() ? " from " + pid.get() : ""):



src/slave/slave.cpp (lines 2792 - 2800)


ditto. separate LOG(WARNING) messages puts the text on separate lines. not 
what you want here.



src/slave/slave.cpp 


how about short-circuiting here when an ack doesn't need to be sent?

//
if (pid.isSome() && pid.get() == UPID()) {
  return;
}



src/slave/slave.cpp (line 3033)


s/status update/sending acknowledgement for status update/



src/slave/slave.cpp (lines 3042 - 3043)


LOG(WARNING) << "Ignoring sending acknowledgement for status update " << 
update
   << " of unknown executor";


- Vinod Kone


On Nov. 21, 2015, 12:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40563/
> ---
> 
> (Updated Nov. 21, 2015, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3476
> https://issues.apache.org/jira/browse/MESOS-3476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the ability for the agent to handle status updates from HTTP 
> based executors. Previously, the existing `statusUpdate` method used to 
> handle status updates sent from slave with `pid == UPID()`, sent from other 
> executors/on behalf of other executors with `pid == Some()`. This change 
> modifies the argument to be `Option`. This ensures that `pid == None()` 
> when set, the ACK is correctly routed to the corresponding HTTP based 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
>   src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
>   src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 
> 
> Diff: https://reviews.apache.org/r/40563/diff/
> 
> 
> Testing
> ---
> 
> make check + would add tests once the https://reviews.apache.org/r/39297/ 
> chain is reviewed.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Anand Mazumdar]

> On Nov. 21, 2015, 12:58 a.m., Vinod Kone wrote:
> > src/slave/http.cpp, lines 318-322
> > 
> >
> > indent by 4 spaces.
> > 
> > also, it's hard to tell that None() is the argument for statusUpdate() 
> > and not createStatusUpdate()

Fixed the indent. Agreed that None() as a argument is a bit hard to 
disambiguate but this is followed elsewhere too. e.g:

https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L4800


> On Nov. 21, 2015, 12:58 a.m., Vinod Kone wrote:
> > src/slave/slave.cpp, line 2764
> > 
> >
> > this will be printed by itself on a new line.
> > 
> > just do;
> > 
> > LOG(INFO) << "Handling status update " << update
> >   << (pid.isSome() ? " from " + pid.get() : ""):

My bad. Fixed.


- Anand


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


On Nov. 21, 2015, 12:21 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40563/
> ---
> 
> (Updated Nov. 21, 2015, 12:21 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-3476
> https://issues.apache.org/jira/browse/MESOS-3476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds the ability for the agent to handle status updates from HTTP 
> based executors. Previously, the existing `statusUpdate` method used to 
> handle status updates sent from slave with `pid == UPID()`, sent from other 
> executors/on behalf of other executors with `pid == Some()`. This change 
> modifies the argument to be `Option`. This ensures that `pid == None()` 
> when set, the ACK is correctly routed to the corresponding HTTP based 
> executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
>   src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
>   src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 
> 
> Diff: https://reviews.apache.org/r/40563/diff/
> 
> 
> Testing
> ---
> 
> make check + would add tests once the https://reviews.apache.org/r/39297/ 
> chain is reviewed.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 40563: Added functionality for handling status updates from HTTP based executors

[Anand Mazumdar]

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

(Updated Nov. 21, 2015, 1:45 a.m.)


Review request for mesos, Ben Mahler, Isabel Jimenez, Marco Massenzio, and 
Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

This change adds the ability for the agent to handle status updates from HTTP 
based executors. Previously, the existing `statusUpdate` method used to handle 
status updates sent from slave with `pid == UPID()`, sent from other 
executors/on behalf of other executors with `pid == Some()`. This change 
modifies the argument to be `Option`. This ensures that `pid == None()` 
when set, the ACK is correctly routed to the corresponding HTTP based executor.


Diffs (updated)
-

  src/slave/http.cpp ce48a0584ab18a8d95dd02619f62df18b2276639 
  src/slave/slave.hpp ec2dfa99e6b553e2bcd82d12db915ae8625075a1 
  src/slave/slave.cpp d1126f00d947fdb4823b0c495335b743254ac7ee 

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


Testing
---

make check + would add tests once the https://reviews.apache.org/r/39297/ chain 
is reviewed.


Thanks,

Anand Mazumdar