mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ian Downes" <ian.dow...@gmail.com>
Subject Re: Review Request 32664: Add port mapping isolator statistics tests
Date Wed, 08 Apr 2015 19:46:12 GMT

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


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