Re: Review Request 38577: Added synchronous validation for Call in Agent

[Vinod Kone]

> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.cpp, line 75
> > 
> >
> > also print status.source()
> 
> Isabel Jimenez wrote:
> We don't have a stringify for this.
> 
> Anand Mazumdar wrote:
> Why not implement the `ostream` operator preferrably in `type_utils.hpp` 
> in a separate review and mark this one as dependent on it ? Otherwise, this 
> error string is hardly of any use to the end-user. What do you think?

+1 to anand's comment. but lets do that in a follow up review. i'll commit this 
chain now.


- Vinod


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


On Oct. 27, 2015, 1:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 1:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.hpp, line 31
> > 
> >
> > why space here?

Added it there after a review comment.


> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.cpp, line 75
> > 
> >
> > also print status.source()

We don't have a stringify for this.


> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/tests/executor_http_api_tests.cpp, line 505
> > 
> >
> > make sure this pattern is consistent everywhere else in this file.

Inversed advise here, killed this and made sure above pattern is consistent 
everywhere.


- Isabel


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


On Oct. 27, 2015, 1:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 1:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 12:23 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

rebase


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt e6169a0 
  src/Makefile.am e797dac 
  src/slave/http.cpp 3f7f71b 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 
  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 27, 2015, 1:20 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

review


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt e6169a0 
  src/Makefile.am e797dac 
  src/slave/http.cpp 3f7f71b 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 
  src/tests/executor_http_api_tests.cpp e429d84 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Vinod Kone]

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

Ship it!



src/slave/validation.hpp (line 19)


__SLAVE_VALIDATION_HPP__



src/slave/validation.hpp (line 31)


why space here?



src/slave/validation.cpp (line 25)


kill new line.



src/slave/validation.cpp (line 65)


space before 'does'



src/slave/validation.cpp (line 75)


also print status.source()



src/tests/executor_http_api_tests.cpp (line 502)


kill this?



src/tests/executor_http_api_tests.cpp (line 504)


make sure this pattern is consistent everywhere else in this file.


- Vinod Kone


On Oct. 27, 2015, 12:23 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 12:23 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

> On Oct. 27, 2015, 1:13 a.m., Vinod Kone wrote:
> > src/slave/validation.cpp, line 75
> > 
> >
> > also print status.source()
> 
> Isabel Jimenez wrote:
> We don't have a stringify for this.

Why not implement the `ostream` operator preferrably in `type_utils.hpp` in a 
separate review and mark this one as dependent on it ? Otherwise, this error 
string is hardly of any use to the end-user. What do you think?


- Anand


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


On Oct. 27, 2015, 1:20 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Oct. 27, 2015, 1:20 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e6169a0 
>   src/Makefile.am e797dac 
>   src/slave/http.cpp 3f7f71b 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
>   src/tests/executor_http_api_tests.cpp e429d84 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 5, 2015, 3:54 p.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt 891f951 
  src/Makefile.am f060998 
  src/slave/http.cpp f9cf7bb 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Oct. 2, 2015, 6:42 p.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt 891f951 
  src/Makefile.am 8aa4566 
  src/slave/http.cpp f9cf7bb 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Sept. 29, 2015, 8 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/Makefile.am 8aa4566 
  src/slave/http.cpp cddc8ad 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Alex Clemmer]

> On Sept. 25, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 501
> > 
> >
> > I think you'll make Alex happy if you also update the corresponding 
> > CMake file(s).
> 
> Isabel Jimenez wrote:
> We don't have cmake support for the agent yet.
> 
> Alex Clemmer wrote:
> I'm pleased to report that we do, actually! Could we please put this line 
> in the `CMakeLists.txt` found in `src/`.

(I should mention that the reason I didn't appear earlier to remind you is 
because this was only checked in a day or so ago.)


- Alex


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


On Sept. 29, 2015, 8 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 8 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp cddc8ad 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Alex Clemmer]

> On Sept. 25, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 501
> > 
> >
> > I think you'll make Alex happy if you also update the corresponding 
> > CMake file(s).
> 
> Isabel Jimenez wrote:
> We don't have cmake support for the agent yet.

I'm pleased to report that we do, actually! Could we please put this line in 
the `CMakeLists.txt` found in `src/`.


- Alex


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


On Sept. 29, 2015, 8 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 8 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp cddc8ad 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Sept. 29, 2015, 8:29 p.m., Anand Mazumdar wrote:
> > src/slave/validation.cpp, line 57
> > 
> >
> > Sorry for the ping pong iterations on this but noticed another missing 
> > validation check for `Call::UPDATE` while working on MESOS-3480.
> > 
> > Can we also check if `TaskState` is not `TASK_STAGING` and return an 
> > validation error if so ?
> > 
> > https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L2838

This introduces a change of behavior, I'll reflect this on the design Doc too.


- Isabel


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


On Sept. 29, 2015, 8:15 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 8:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp f9cf7bb 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Sept. 29, 2015, 9:25 p.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt 891f951 
  src/Makefile.am 8aa4566 
  src/slave/http.cpp f9cf7bb 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

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


Thanks Isabel, this looks pretty neat now. Just some minor cleanup comments.


src/slave/validation.cpp (line 58)


Nit: const TaskStatus& status = call.update().status();



src/slave/validation.cpp (line 65)


s/in Update/in TaskStatus



src/slave/validation.cpp (lines 70 - 77)


Should we be explicitly validating that the source is always 
SOURCE_EXECUTOR i.e. something like ?

if (state.source() != TaskStatus::SOURCE_EXECUTOR) {
// Failed Validation
}

...

if (status.state() == TASK_STAGING) {
// Failed Validation
}


- Anand Mazumdar


On Sept. 29, 2015, 9:25 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 9:25 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp f9cf7bb 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Sept. 29, 2015, 8:15 p.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

Added validation.cpp to CMakelist


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/CMakeLists.txt 891f951 
  src/Makefile.am 8aa4566 
  src/slave/http.cpp f9cf7bb 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

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



src/slave/validation.cpp (line 57)


Sorry for the ping pong iterations on this but noticed another missing 
validation check for `Call::UPDATE` while working on MESOS-3480.

Can we also check if `TaskState` is not `TASK_STAGING` and return an 
validation error if so ?

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


- Anand Mazumdar


On Sept. 29, 2015, 8:15 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 8:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 891f951 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp f9cf7bb 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Sept. 25, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 501
> > 
> >
> > I think you'll make Alex happy if you also update the corresponding 
> > CMake file(s).
> 
> Isabel Jimenez wrote:
> We don't have cmake support for the agent yet.
> 
> Alex Clemmer wrote:
> I'm pleased to report that we do, actually! Could we please put this line 
> in the `CMakeLists.txt` found in `src/`.
> 
> Alex Clemmer wrote:
> (I should mention that the reason I didn't appear earlier to remind you 
> is because this was only checked in a day or so ago.)

Great news!


- Isabel


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


On Sept. 29, 2015, 8 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 29, 2015, 8 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 8aa4566 
>   src/slave/http.cpp cddc8ad 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

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



src/slave/validation.cpp (line 52)


Can you also ensure that the `ExecutorID` sent as part of the 
`Call::Update::TaskStatus` message matches the one being sent as part of the 
Call message if it is set ?


- Anand Mazumdar


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Sept. 25, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/slave/validation.hpp, line 36
> > 
> >
> > Can you please add tests in this patch?

Adding tests in a separate dependant patch.


- Isabel


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


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Vinod Kone]

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



src/Makefile.am (line 501)


I think you'll make Alex happy if you also update the corresponding CMake 
file(s).



src/slave/validation.hpp (line 36)


Can you please add tests in this patch?



src/slave/validation.cpp (lines 35 - 43)


I would swap the order of these.


- Vinod Kone


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Sept. 25, 2015, 6:52 p.m., Vinod Kone wrote:
> > src/Makefile.am, line 501
> > 
> >
> > I think you'll make Alex happy if you also update the corresponding 
> > CMake file(s).

We don't have cmake support for the agent yet.


- Isabel


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


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Sept. 22, 2015, 9:46 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs
-

  src/Makefile.am e224060 
  src/slave/http.cpp 12a4d39 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

(Updated Sept. 22, 2015, 9:43 a.m.)


Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


Changes
---

review comments


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs (updated)
-

  src/Makefile.am e224060 
  src/slave/http.cpp 12a4d39 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38618, 38577]

All tests passed.

- Mesos ReviewBot


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

> On Sept. 22, 2015, 1:08 a.m., Anand Mazumdar wrote:
> > src/slave/validation.hpp, line 28
> > 
> >
> > newline before. I am expecting other people to add slave validation 
> > code in this file too in the future. So separating `namespace 
> > executor/call` by a newline would be a good idea.

I don't think is very relevant for this patch, but I agree that a separation 
can help future changes.


> On Sept. 22, 2015, 1:08 a.m., Anand Mazumdar wrote:
> > src/slave/validation.hpp, line 32
> > 
> >
> > What do you think about going ahead and implementing some unit tests ?
> > 
> > You can create another patch if you would like for the tests. But in 
> > general, it's good practice to have tests in most cases even for this 
> > trivial validation code :)
> > 
> > I guess we would need the following tests in 
> > `src/tests/executor_http_api_tests.cpp` ?
> > - Missing Executor/Framework Id.
> > - Invalid call message that is not initialized.
> > - Invalid call message that does not have Subscribe/Update/Message but 
> > has the corresponding type set.

Getting a patch for tests separately.


- Isabel


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


On Sept. 22, 2015, 9:46 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 22, 2015, 9:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Review Request 38577: Added synchronous validation for Call in Agent

[Isabel Jimenez]

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

Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

Added validation for Call protobuf message in Agent /api/v1/executor endpoint.


Diffs
-

  src/Makefile.am e224060 
  src/slave/http.cpp 12a4d39 
  src/slave/validation.hpp PRE-CREATION 
  src/slave/validation.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38577: Added synchronous validation for Call in Agent

[Anand Mazumdar]

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


LGTM, thanks for working on this Isabel ! Waiting for some unit-tests to 
bless/approve the implementation code before giving this a Ship It.


src/slave/validation.hpp (line 18)


Missing header include guards ?



src/slave/validation.hpp (line 28)


newline before. I am expecting other people to add slave validation code in 
this file too in the future. So separating `namespace executor/call` by a 
newline would be a good idea.



src/slave/validation.hpp (line 32)


What do you think about going ahead and implementing some unit tests ?

You can create another patch if you would like for the tests. But in 
general, it's good practice to have tests in most cases even for this trivial 
validation code :)

I guess we would need the following tests in 
`src/tests/executor_http_api_tests.cpp` ?
- Missing Executor/Framework Id.
- Invalid call message that is not initialized.
- Invalid call message that does not have Subscribe/Update/Message but has 
the corresponding type set.



src/slave/validation.cpp (line 25)


newline before. Same reason as before.


- Anand Mazumdar


On Sept. 21, 2015, 11:23 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38577/
> ---
> 
> (Updated Sept. 21, 2015, 11:23 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-2906
> https://issues.apache.org/jira/browse/MESOS-2906
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for Call protobuf message in Agent /api/v1/executor endpoint.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e224060 
>   src/slave/http.cpp 12a4d39 
>   src/slave/validation.hpp PRE-CREATION 
>   src/slave/validation.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38577/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>