Re: Review Request 32133: Refactor port isolator tests to break out helper functions for testing of bandwidth limit statistics

[Ian Downes]

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



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124368>

    const char foobar[]



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124369>

    We write comments as complete sentences.
    s/use/Use
    Add punctuation, end with a full stop.



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124374>

    punctuation.
    single space after punctuation.



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124375>

    Why an empty JSON object rather than a Try<JSON::Object>?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124388>

    Perhaps pass in the path to the helper function or just the launcher_dir 
rather than all the flags?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124376>

    s/log to slave process/log to the calling process



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124372>

    Is this output necessary or was it debug?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124379>

    This would be more idiomatic:
    
    Try<JSON::Object> parse = JSON::parse<>();
    if (parse.isError()) {
      return Error(...);
    }
    
    return parse.get();



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124377>

    Seems like this should return an Error()?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124378>

    Is this output necessary or was it debug?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124382>

    Use Duration rather than time_t.
    
    This is more idiomatic, e.g., see isolator_tests.cpp, but see comment later 
about returning a Future<bool>.
    
    bool waitForFileExists(
      const string& path,
      const Duration& timeout)
    {
      Duration waited = Duration::zero();
      do {
        if (os::exists(path)) {
          return true;
        }
      
        os::sleep(Milliseconds(50));
        waited += Milliseconds(50);
      } while (waited < timeout);
      
      return false;
    }



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124390>

    I don't see the value in pulling these out as separate functions compared 
to the original code? _Perhaps_ a single function that took the pXX to find?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124393>

    These aren't 'default' and 'alternate', they are separate ports for 
container1 and for container2 of the same type?



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124397>

    If you're fixing up the naming for ports, then it seems like the implicit 
rules for assigning these ports and the container{1,2} ports should also be 
fixe/documented.



src/tests/port_mapping_tests.cpp
<https://reviews.apache.org/r/32133/#comment124400>

    Better would be if waitForFileCreation() returned a Future<bool> that was 
ready when the path existed. Then you could do AWAIT_READY() and the 
(configurable) timeout would be in the caller waiting on the future, not in the 
helper.
    
    I could imagine this would be generally useful; consider adding this to 
stout?


- Ian Downes


On March 16, 2015, 4:28 p.m., Paul Brett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/32133/
> -----------------------------------------------------------
> 
> (Updated March 16, 2015, 4:28 p.m.)
> 
> 
> Review request for mesos and Ian Downes.
> 
> 
> Bugs: mesos-2332
>     https://issues.apache.org/jira/browse/mesos-2332
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Refactor port isolator tests to break out helper functions for testing of 
> bandwidth limit statistics
> 
> 
> Diffs
> -----
> 
>   src/tests/port_mapping_tests.cpp 82f98a47fa374fda13b0be76b07ccc03174a7b96 
> 
> Diff: https://reviews.apache.org/r/32133/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>