> On Sept. 22, 2015, 1:08 a.m., Anand Mazumdar wrote: > > src/slave/validation.hpp, line 28 > > <https://reviews.apache.org/r/38577/diff/1/?file=1078641#file1078641line28> > > > > 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 > > <https://reviews.apache.org/r/38577/diff/1/?file=1078641#file1078641line32> > > > > 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 > >
