Re: Review Request 32664: Add port mapping isolator statistics tests

[Ian Downes]

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


A few major concerns here before I do a proper review:
1. Did you consider using iperf3's library? If considered, why not. If no 
considered, can we?
2. There's a lot of use of shell and other programs here, e.g., perl. Hopefully 
this can be avoided by (1).
3. There's places where the shell is used for signaling, sleeping etc. This 
should all be done in code unless necessary. 
4. We use camel case and generally full words, e.g., percentageDropped rather 
than pdropped.
5. Look into using the stout constructs like Option<string> rather than using 
empty strings to signify something optional.

Some of these I've created issues just to highlight specific examples.


src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128733>

    Did you consider using iperf3 which makes all features available through a 
library?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128728>

    our style is
    ```cpp
    IperfServer(const string& _host, ...)
    : ...
      host(_host)
      ...
    ```



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128734>

    s/pdropped/percentageDropped?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128735>

    ditto



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128736>

    ditto



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128729>

    \n



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128730>

    Option<string> pidFile?
    
    ```cpp
    if (pidFile.isNone()) {
       ...
    }
    ```



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128737>

    that's a scary regex! I'm guessing a lot of this would be vastly simpler if 
we used the iper3 library. Did you investigate that?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128740>

    Why do this in a shell? why not do this in code - signal the pid as desired.



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128750>

    base-10 or base-2?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128745>

    style is to just use Try<>.get() rather than extracting:
    ```cpp
    Try<Isolator*> isolator = PortMappingIsolatorProcess::create(flags);
    CHECK_SOME(isolator);
    
    ...
    Future<Option<CommandInfo> > preparation1 =
          isolator.get()->prepare(containerId, executorInfo, dir1.get(), 
None());
    ```



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128747>

    s/lnch/launcher/



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32664/#comment128746>

    Why does it need to be random if we're inside a temporary directory sandbox?


- Ian Downes


On April 1, 2015, 8:45 a.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32664/
> -----------------------------------------------------------
> 
> (Updated April 1, 2015, 8:45 a.m.)
> 
> 
> Review request for mesos, Chi Zhang, Ian Downes, and Cong Wang.
> 
> 
> Bugs: mesos-2332
>     https://issues.apache.org/jira/browse/mesos-2332
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Add port mapping isolator statistics tests
> 
> 
> Diffs
> -----
> 
>   src/tests/port_mapping_tests.cpp f4124c3e880e043729579a829e1057727741d131 
> 
> Diff: https://reviews.apache.org/r/32664/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>