----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/54297/#review157901 -----------------------------------------------------------
Modulo Jie's comments. Did an initial pass. Looks pretty good! Mostly minor comments around style. The only major one being that we can simplify the input handler greatly by introducing validation overloads. See comsment later. src/slave/containerizer/mesos/io/switchboard.cpp (line 17) <https://reviews.apache.org/r/54297/#comment228544> You might not need this after Jie's comment? src/slave/containerizer/mesos/io/switchboard.cpp (line 863) <https://reviews.apache.org/r/54297/#comment228524> Nit: Quotes before 'ATTACH_CONTAINER_INPUT' src/slave/containerizer/mesos/io/switchboard.cpp (lines 869 - 871) <https://reviews.apache.org/r/54297/#comment228523> Kill this since you assert on this a couple of lines later? src/slave/containerizer/mesos/io/switchboard.cpp (line 880) <https://reviews.apache.org/r/54297/#comment228525> 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`? src/slave/containerizer/mesos/io/switchboard.cpp (line 884) <https://reviews.apache.org/r/54297/#comment228526> Nit: Can we be consistent with the agent validation strings here: "Expecting 'agent_container_input.type' to be 'PROCESS_IO'" Check `src/slave/validation.cpp`. src/slave/containerizer/mesos/io/switchboard.cpp (line 890) <https://reviews.apache.org/r/54297/#comment228527> "Expecting 'attach_container_input.process_io' to be present" src/slave/containerizer/mesos/io/switchboard.cpp (line 895) <https://reviews.apache.org/r/54297/#comment228528> const reference src/slave/containerizer/mesos/io/switchboard.cpp (line 900) <https://reviews.apache.org/r/54297/#comment228529> "Expecting 'attach_container_input.process_io.type' to be present" src/slave/containerizer/mesos/io/switchboard.cpp (lines 904 - 905) <https://reviews.apache.org/r/54297/#comment228530> Can you just check for `UNKNOWN` here? Also, the error string can be "'attach_container_input.process_io.type' is unknown" src/slave/containerizer/mesos/io/switchboard.cpp (line 911) <https://reviews.apache.org/r/54297/#comment228538> 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? src/slave/containerizer/mesos/io/switchboard.cpp (lines 915 - 916) <https://reviews.apache.org/r/54297/#comment228539> Can you make the error string consistent here based on previous comments? src/slave/containerizer/mesos/io/switchboard.cpp (lines 921 - 922) <https://reviews.apache.org/r/54297/#comment228540> Can you make the error string consistent here based on previous comments? src/slave/containerizer/mesos/io/switchboard.cpp (line 929) <https://reviews.apache.org/r/54297/#comment228541> Nit: Missing quotes before 'TTY_INFO' src/slave/containerizer/mesos/io/switchboard.cpp (line 945) <https://reviews.apache.org/r/54297/#comment228535> Const reference? src/slave/containerizer/mesos/io/switchboard.cpp (line 989) <https://reviews.apache.org/r/54297/#comment228542> s/our/the s/appropriately// src/slave/containerizer/mesos/io/switchboard.cpp (line 992) <https://reviews.apache.org/r/54297/#comment228543> s/simply// src/slave/containerizer/mesos/io/switchboard.cpp (line 997) <https://reviews.apache.org/r/54297/#comment228531> Can you use explicit capture here? Also s/Future<Nothing>/Future<Nothing>& src/slave/containerizer/mesos/io/switchboard.cpp (lines 1003 - 1010) <https://reviews.apache.org/r/54297/#comment228532> You should be able to combine these: ```cpp failure = Failure("Failed writing to stdin:" " " + (future.isFailed() ? future.failure() : "discarded"); ``` src/slave/containerizer/mesos/io/switchboard.cpp (line 1021) <https://reviews.apache.org/r/54297/#comment228533> Can you use explicit capture here? src/slave/containerizer/mesos/io/switchboard.cpp (line 1022) <https://reviews.apache.org/r/54297/#comment228534> s/connections/input connections - Anand Mazumdar On Dec. 3, 2016, 2:07 a.m., Kevin Klues wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/54297/ > ----------------------------------------------------------- > > (Updated Dec. 3, 2016, 2:07 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 > 594c37e531c5b26599989a126aede56954d460fa > > Diff: https://reviews.apache.org/r/54297/diff/ > > > Testing > ------- > > This is not tesedt yet. I plan to write a test and update this patch soon. > Just wanted to get a preliminary patch out there for review. > > > Thanks, > > Kevin Klues > >
