Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 5, 2016, 7:09 a.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Rebased on master.


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 (updated)
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
d5211b98616e72a27ca6b472a5ee83505c227f22 
  src/tests/containerizer/io_switchboard_tests.cpp 
ebf9bc75b8a256847a507954615241cff1da4dc3 

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


Testing
---

make -j check


Thanks,

Kevin Klues

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 4, 2016, 7:17 p.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Updated to address newest comments.


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 (updated)
-

  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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

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


Fix it, then Ship it!




I would file a follow up issue to add tests for the validation code itself.


src/slave/containerizer/mesos/io/switchboard.cpp (lines 492 - 493)


How about:

```cpp
// TODO(klueska): Move this to `src/slave/validation.hpp` and make the 
agent validate all the calls before forwarding them to the switchboard.




src/slave/containerizer/mesos/io/switchboard.cpp (lines 847 - 849)


Kill this, you already have an explicit `CHECK` for this before you invoke 
this.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 852 - 866)


For this handler, it should *only* validate that the second record is a 
`ProcessIO` record. It shouldn't validate the other types and just return an 
error if the record is of some other type.

Can you just do:

```cpp
case agent::Call::AttachContainerInput::UNKNOWN:
case agent::Call::AttachContainerInput::CONTAINER_ID: {
  return Error("Expecting 'attach_container_input.type' to be 'PROCESS_IO'"
   " instead of: '" + call.type() + "'");
```



src/slave/containerizer/mesos/io/switchboard.cpp (lines 934 - 936)


We rely on the compiler failing if a `condition` is missed in the case 
statements `-Wswitch`.

Kill this and just have `UNREACHABLE()` in the outer scope.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 939 - 941)


Kill this and replace with unreachable outside.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 984 - 985)


Kill this invariant check.

The agent does not validate if the second record type is `PROCESS_IO`. We 
need to validate it in the handler itself for now as per our earlier discussion.



src/slave/containerizer/mesos/io/switchboard.cpp (lines 1044 - 1046)


Kill this and just have a `UNREACHABLE()`.


- Anand Mazumdar


On Dec. 4, 2016, 6:55 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, 6:55 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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 4, 2016, 6:55 a.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Updated based on Anand's comments.


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 (updated)
-

  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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > 
> >
> > 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
> > 
> >
> > 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` 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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

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



Just a few more minor things to clean up.


src/slave/containerizer/mesos/io/switchboard.cpp (lines 832 - 833)


hmm, the `PROCESS_IO` validation can be done by the agent itself i.e., it 
seems to be independent of the business logic of the handler.

Can you add a `TODO` in the function declaration to move this logic to the 
agent in `src/slave/validation.hpp` if you don't want to do this now?

Also, can you change the method signature to be:
```cpp
Option IOSwitchboardServerProcess::validate(
const agent::Call::AttachContainerInput& call)
```



src/slave/containerizer/mesos/io/switchboard.cpp (lines 835 - 840)


With the change in method signature, move these to the handler (call site) 
itself. (L964)



src/slave/containerizer/mesos/io/switchboard.cpp (lines 863 - 925)


There seem perfect to use the switch statement on `type()` instead of the 
`if else` statements?



src/slave/containerizer/mesos/io/switchboard.cpp (line 974)


Any reasons for not using switch statements here? If not, can you use them 
here?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 1012 - 1014)


Nit: Should fit in one line, no?



src/tests/containerizer/io_switchboard_tests.cpp (line 301)


Nice Test!



src/tests/containerizer/io_switchboard_tests.cpp (line 303)


s/only//


- Anand Mazumdar


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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

---
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.


Changes
---

Updated to remove circular dependency in reviews.


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 (updated)
-

  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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > 
> >
> > 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?

I've just updated the patch again with taking a stab at this. Let me know what 
you think.


- Kevin


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


On Dec. 4, 2016, 12:30 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:30 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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 4, 2016, 12:30 a.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Updated to address the rest of Anand's comments.


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 (updated)
-

  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
---

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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 4, 2016, 12:30 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 (updated)
---

make -j check


Thanks,

Kevin Klues

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

(Updated Dec. 3, 2016, 11:45 p.m.)


Review request for mesos, Anand Mazumdar and Jie Yu.


Changes
---

Addressed most of Anand's comments (still waiting on some feedback for a few).


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 (updated)
-

  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
---

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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 885
> > 
> >
> > 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`?

Why doesn't the server validate this (it could, and maybe should)? First 
message is `CONTAINER_ID` type, all others are `PROCESS_IO` type.


> On Dec. 3, 2016, 8:36 p.m., Anand Mazumdar wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, line 916
> > 
> >
> > 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?

Where would I put these? In `src/slave/validation.cpp` namespaced under 
`io_switchboard::call`, etc. Or maybe just local to my class?


- Kevin


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


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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

---
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)


You might not need this after Jie's comment?



src/slave/containerizer/mesos/io/switchboard.cpp (line 863)


Nit: Quotes before 'ATTACH_CONTAINER_INPUT'



src/slave/containerizer/mesos/io/switchboard.cpp (lines 869 - 871)


Kill this since you assert on this a couple of lines later?



src/slave/containerizer/mesos/io/switchboard.cpp (line 880)


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)


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)


"Expecting 'attach_container_input.process_io' to be present"



src/slave/containerizer/mesos/io/switchboard.cpp (line 895)


const reference



src/slave/containerizer/mesos/io/switchboard.cpp (line 900)


"Expecting 'attach_container_input.process_io.type' to be present"



src/slave/containerizer/mesos/io/switchboard.cpp (lines 904 - 905)


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)


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)


Can you make the error string consistent here based on previous comments?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 921 - 922)


Can you make the error string consistent here based on previous comments?



src/slave/containerizer/mesos/io/switchboard.cpp (line 929)


Nit: Missing quotes before 'TTY_INFO'



src/slave/containerizer/mesos/io/switchboard.cpp (line 945)


Const reference?



src/slave/containerizer/mesos/io/switchboard.cpp (line 989)


s/our/the
s/appropriately//



src/slave/containerizer/mesos/io/switchboard.cpp (line 992)


s/simply//



src/slave/containerizer/mesos/io/switchboard.cpp (line 997)


Can you use explicit capture here?

Also s/Future/Future&



src/slave/containerizer/mesos/io/switchboard.cpp (lines 1003 - 1010)


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)


Can you use explicit capture here?



src/slave/containerizer/mesos/io/switchboard.cpp (line 1022)

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54274, 54296, 54297]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

---
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.


Changes
---

Updated to return proper response types for all failure cases.


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 (updated)
-

  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

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Jie Yu]

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




src/slave/containerizer/mesos/io/switchboard.cpp (lines 947 - 954)


Let's put this into stout/posix/os.hpp and call it 
`os::setWindowSize(unsigned short row, unsigned short column)`


- Jie Yu


On Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> ---
> 
> (Updated Dec. 2, 2016, 6: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 
> 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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Anand Mazumdar]

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



Let me commit the attach output review/`serv()` to use the streaming decoder 
first and then keep reviewing this. (We can't commit this anyways without tests)

- Anand Mazumdar


On Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> ---
> 
> (Updated Dec. 2, 2016, 6: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 
> 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
> 
>

Re: Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54274, 54296, 54297]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker_build.sh

- Mesos ReviewBot


On Dec. 2, 2016, 6:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54297/
> ---
> 
> (Updated Dec. 2, 2016, 6: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 
> 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
> 
>

Review Request 54297: Added support for ATTACH_CONTAINER_INPUT to the io switchboard.

[Kevin Klues]

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

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