mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kevin Klues <klue...@gmail.com>
Subject Re: Review Request 54293: Supported TTY in I/O switchboard.
Date Mon, 05 Dec 2016 02:02:52 GMT

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




src/slave/containerizer/mesos/io/switchboard.hpp (line 126)
<https://reviews.apache.org/r/54293/#comment228606>

    It's somewhat arbitrary, but can we make this the second to last parameter, in order to
group the booleans together? If so, can we do it throughout?



src/slave/containerizer/mesos/io/switchboard.hpp (line 178)
<https://reviews.apache.org/r/54293/#comment228607>

    s/termial/terminal



src/slave/containerizer/mesos/io/switchboard.cpp (line 247)
<https://reviews.apache.org/r/54293/#comment228608>

    Can you add a comment here about the difference between the two hashsets? Reading top
to bottom, it's not clear exactly what each one represents just by looking at the name of
the variable.



src/slave/containerizer/mesos/io/switchboard.cpp (line 258)
<https://reviews.apache.org/r/54293/#comment228609>

    s/the pseudo/a pseudo



src/slave/containerizer/mesos/io/switchboard.cpp (line 277)
<https://reviews.apache.org/r/54293/#comment228610>

    Does it make sense to eventually move this function into stout?



src/slave/containerizer/mesos/io/switchboard.cpp (line 283)
<https://reviews.apache.org/r/54293/#comment228611>

    Does it make sense to eventually move this function into stout?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 388 - 391)
<https://reviews.apache.org/r/54293/#comment228612>

    I guess you don't need to set the other flags anymore because you rely on their default
values now?



src/slave/containerizer/mesos/io/switchboard.cpp (lines 439 - 441)
<https://reviews.apache.org/r/54293/#comment228613>

    Can you add a comment about why we do this? When I first looked at it, it wasn't obvious
why it was necessary, but I see why it's needed later on (i.e. for future failures).



src/slave/containerizer/mesos/io/switchboard.cpp (lines 749 - 750)
<https://reviews.apache.org/r/54293/#comment228614>

    Can you add a quick comment about why we don't need to redirect to `stderr` if we have
a tty set up.


- Kevin Klues


On Dec. 4, 2016, 9:21 p.m., Jie Yu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54293/
> -----------------------------------------------------------
> 
> (Updated Dec. 4, 2016, 9:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kevin Klues.
> 
> 
> Bugs: MESOS-6470
>     https://issues.apache.org/jira/browse/MESOS-6470
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Supported TTY in I/O switchboard.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/containerizer.proto 33b4c231cc88a048f3de8ab4b2e0e9eb2140132d 
>   src/slave/containerizer/mesos/containerizer.cpp a7e2665d4c64b7b322d4ae8d5441e8e777236c59

>   src/slave/containerizer/mesos/io/switchboard.hpp 23c66bb2b05310a12fec24a5e5eebddd370ba0f0

>   src/slave/containerizer/mesos/io/switchboard.cpp 778367a268ec350ed438bc9fe9d359d63bdb5503

>   src/slave/containerizer/mesos/io/switchboard_main.cpp 5800217d3562a53218d966c57a14e8b768643c34

>   src/slave/containerizer/mesos/launch.cpp d78ca4df694712e60a15f1795878653eb5e0414e 
>   src/tests/containerizer/io_switchboard_tests.cpp d82f22b5c9a8d177577db3aeaae9e571e7e2fb80

> 
> Diff: https://reviews.apache.org/r/54293/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message