> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/mesos/io/switchboard.cpp, line 916 > > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line916> > > > > hmm, this message is a bit mis-leading given that you only start > > processing the control message from L953. This is just validating the > > control message itself. > > > > How about you introduce validation overloads. This would simplify your > > call sites a lot i.e., you won't have to manually set `response` for every > > error case. An example here: > > > > ```cpp > > // Validate the call. > > ...... error = validate(...); > > if (error.isSome()) { > > return BadRequest(error.get()); > > } > > > > // Validate the 'PROCESS_IO' message. > > .... > > > > // Validate the control message. > > .... > > > > // Process the control message. > > ... > > > > // Validate the data message. > > ... > > > > // Process the data message. > > > > ``` > > > > What do you think? > > Kevin Klues wrote: > Where would I put these? In `src/slave/validation.cpp` namespaced under > `io_switchboard::call`, etc. Or maybe just local to my class? > > Kevin Klues wrote: > I've just updated the patch again with taking a stab at this. Let me know > what you think.
Marking this as fixed, added comments on this. > On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote: > > src/slave/containerizer/mesos/io/switchboard.cpp, line 885 > > <https://reviews.apache.org/r/54297/diff/2/?file=1575106#file1575106line885> > > > > Can you add a comment here for posterity as to why these are not > > invariant checks i.e., the agent does not validate if the next message > > should be `PROCESS_IO`? > > Kevin Klues wrote: > Why doesn't the server validate this (it could, and maybe should)? First > message is `CONTAINER_ID` type, all others are `PROCESS_IO` type. This is a tech-debt that we should clean up eventually. See my latest comment around adding a `TODO` to move all the validation logic to the agent itself. The `transform()` helper that the agent uses currently returns `Future<Nothing>` which in turn hides the actual error. Hence, all these validation errors would become `InternalServerError` for the client instead of `BadRequest`. Once we switch to using `process::loop()` (it was not there when the agent handler was being worked on), we would have similar logic to the one you have, to propogate the actual error back to the client. We have to do so soon since `transform()` seems to be plagued by the unbounded future memory growth issue that BenH encountered. I am dropping this issue in favor of the recent issue I raised to add a `TODO` to move the validation to the agent itself. - Anand ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54297/#review157901 ----------------------------------------------------------- On Dec. 4, 2016, 12:54 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54297/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2016, 12:54 a.m.) > > > Review request for mesos, Anand Mazumdar and Jie Yu. > > > Bugs: MESOS-6467 > https://issues.apache.org/jira/browse/MESOS-6467 > > > Repository: mesos > > > Description > ------- > > Added support for ATTACH_CONTAINER_INPUT to the io switchboard. > > > Diffs > ----- > > src/slave/containerizer/mesos/io/switchboard.cpp > 778367a268ec350ed438bc9fe9d359d63bdb5503 > src/tests/containerizer/io_switchboard_tests.cpp > d82f22b5c9a8d177577db3aeaae9e571e7e2fb80 > > Diff: https://reviews.apache.org/r/54297/diff/ > > > Testing > ------- > > make -j check > > > Thanks, > > Kevin Klues > >
