Return-Path: 
 <dev-return-43128-apmail-mesos-dev-archive=mesos.apache.org@mesos.apache.org>
X-Original-To: apmail-mesos-dev-archive@www.apache.org
Delivered-To: apmail-mesos-dev-archive@www.apache.org
Received: from mail.apache.org (hermes.apache.org [140.211.11.3])
	by minotaur.apache.org (Postfix) with SMTP id A44491784F
	for <apmail-mesos-dev-archive@www.apache.org>;
 Wed,  8 Apr 2015 19:46:20 +0000 (UTC)
Received: (qmail 28243 invoked by uid 500); 8 Apr 2015 19:46:15 -0000
Delivered-To: apmail-mesos-dev-archive@mesos.apache.org
Received: (qmail 28173 invoked by uid 500); 8 Apr 2015 19:46:15 -0000
Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm
Precedence: bulk
List-Help: <mailto:dev-help@mesos.apache.org>
List-Unsubscribe: <mailto:dev-unsubscribe@mesos.apache.org>
List-Post: <mailto:dev@mesos.apache.org>
List-Id: <dev.mesos.apache.org>
Reply-To: dev@mesos.apache.org
Delivered-To: mailing list dev@mesos.apache.org
Received: (qmail 28154 invoked by uid 99); 8 Apr 2015 19:46:15 -0000
Received: from reviews-vm.apache.org (HELO reviews.apache.org) (140.211.11.40)
    by apache.org (qpsmtpd/0.29) with ESMTP; Wed, 08 Apr 2015 19:46:15 +0000
Received: from reviews.apache.org (localhost [127.0.0.1])
	by reviews.apache.org (Postfix) with ESMTP id BFBD41D7565;
	Wed,  8 Apr 2015 19:46:12 +0000 (UTC)
Content-Type: multipart/alternative;
 boundary="===============4852236267205483511=="
MIME-Version: 1.0
Subject: Re: Review Request 32664: Add port mapping isolator statistics tests
From: "Ian Downes" <ian.downes@gmail.com>
To: "Cong Wang" <cwang@twopensource.com>, "Ian Downes" <ian.downes@gmail.com>,
 "Chi Zhang" <chzhcn88@gmail.com>
Cc: "mesos" <dev@mesos.apache.org>, "Paul Brett" <paul_b@twopensource.com>
Date: Wed, 08 Apr 2015 19:46:12 -0000
Message-ID: <20150408194612.5637.4244@reviews.apache.org>
X-ReviewBoard-URL: https://reviews.apache.org/
Auto-Submitted: auto-generated
Sender: "Ian Downes" <noreply@reviews.apache.org>
X-ReviewGroup: mesos
X-ReviewRequest-URL: https://reviews.apache.org/r/32664/
X-Sender: "Ian Downes" <noreply@reviews.apache.org>
References: <20150401154515.16792.39899@reviews.apache.org>
In-Reply-To: <20150401154515.16792.39899@reviews.apache.org>
Reply-To: "Ian Downes" <ian.downes@gmail.com>
X-ReviewRequest-Repository: mesos

--===============4852236267205483511==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


--===============4852236267205483511==--