mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 51715: Added a parallel gtest runner.
Date Mon, 19 Sep 2016 14:36:02 GMT


> On Sept. 15, 2016, 3:16 a.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, lines 36-37
> > <https://reviews.apache.org/r/51715/diff/1/?file=1494165#file1494165line36>
> >
> >     What is a shard in this context?

I introduce this term in the script docstring now.


> On Sept. 15, 2016, 3:16 a.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, line 63
> > <https://reviews.apache.org/r/51715/diff/1/?file=1494165#file1494165line63>
> >
> >     I would prefer to see this function inline under the `__main__` directive below,
and instead have a helper function for parsing the args instead of inlining that content as
done below.

My guideline here was to have functionality accessing global scope at global scope, and isolated
pieces in functions. I understand that your suggestion is more along the lines of the general
Python style, so I aligned the code with that now.


> On Sept. 15, 2016, 3:16 a.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, line 70
> > <https://reviews.apache.org/r/51715/diff/1/?file=1494165#file1494165line70>
> >
> >     At first glance it's not clear to me what this function is used for. Can you
add a more descriptive comment about what this is used for?
> >     
> >     i.e. explain how this is passed to each  subcommand that gets spawned asynchronously
in order to pass it the proper options

I extended the docstring a bit, please let me know if you believe it still needs further explanation.


> On Sept. 15, 2016, 3:16 a.m., Kevin Klues wrote:
> > support/mesos-gtest-runner.py, line 154
> > <https://reviews.apache.org/r/51715/diff/1/?file=1494165#file1494165line154>
> >
> >     How did you come up with this default number of jobs? I would think we should
spawn slightly *less* jobs than the number of CPUs on the machine (not 1.5 times more).
> >     
> >     Also it's not a big deal since we only have one constant for now, but I would
prefer to move this line to the top of the file instead of in line here (e.g. similar to how
we keep all constants at the top of the file in support/generate-endpoint-help.py). If we
keep it here, I would add a newline after it.

Very rarely will our jobs make full use of a single let alone multiple CPUs. In fact it seems
that a large fraction of the test execution time is spent waiting for messages to arrive.
I found that slightly oversubscribing the machine let to better utilization, at least on my
machine with 8 virtual cores.


- Benjamin


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


On Sept. 19, 2016, 4:35 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51715/
> -----------------------------------------------------------
> 
> (Updated Sept. 19, 2016, 4:35 p.m.)
> 
> 
> Review request for mesos, Kevin Klues and Till Toenshoff.
> 
> 
> Bugs: MESOS-6140
>     https://issues.apache.org/jira/browse/MESOS-6140
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This adds `b814987b28cfb65a7c9635c83399545e423e690a` of
> https://github.com/bbannier/gtest-shrdr.
> 
> 
> Diffs
> -----
> 
>   support/mesos-gtest-runner.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51715/diff/
> 
> 
> Testing
> -------
> 
> Tested with e.g., `stout-tests`
> 
>     $ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests
>     ............
>     [PASS]
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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