> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 256
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line256>
> >
> >     Why not await ready? Here and below :)

hmm because when we receive status 'running' from task, then we are pretty sure 
that Resource Estimator/QoS Controller is initialized (: or rather.. we want to 
make sure (:


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/mesos.hpp, lines 188-201
> > <https://reviews.apache.org/r/35157/diff/3/?file=980316#file980316line188>
> >
> >     The StartSlave overload is growing a bit out of hand. Let's create a 
> > JIRA so we get those consolidated somehow. Can you add the ticket as a 
> > comment? :)

MESOS-2833


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 209
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line209>
> >
> >     Can you inline this below? (Is it used other places?) If not, should we 
> > const it? Here and below

Added inline if possible, otherwise const. Added const also for the other 
variables which are not being changed during test.


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, lines 234-238
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line234>
> >
> >     Have you looked into, whether you can use createTask() instead?
> >     
> >     Here and below

Good idea - mimized from e.g master_test.


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 550
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line550>
> >
> >     We tend to use the shorter form, if there is no ambguity (controller 
> > instead of 'qoSController' :) I'll leave that up to you, if you want to 
> > change it

+1 

Done.


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 552
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line552>
> >
> >     Let's introduce a 'using std::list;' above :)

Actually - it was (: But i did not realize that, thx.


> On June 8, 2015, 10:38 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 618
> > <https://reviews.apache.org/r/35157/diff/3/?file=980318#file980318line618>
> >
> >     Let's get the operator overload wired up and do ASSERT_EQ() :)

Done - in type_utils


- Bartek


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


On June 9, 2015, 12:17 a.m., Bartek Plotka wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35157/
> -----------------------------------------------------------
> 
> (Updated June 9, 2015, 12:17 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added tests for QoS Controller and ResourceEstimator fetching ResoruceUsage 
> from ResourceMonitor.
> 
> This is the unit test for https://reviews.apache.org/r/34980/ and 
> https://reviews.apache.org/r/35164/
> 
> 
> Diffs
> -----
> 
>   include/mesos/type_utils.hpp 52380c2461841026ee492797b4d8081f944f7b7b 
>   src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b 
>   src/tests/mesos.hpp 087953d6bc716f11c315a0736f06f712d7f69417 
>   src/tests/mesos.cpp dff45b0d3bf9ef53f19575ab3d90a0b223755d6a 
>   src/tests/oversubscription_tests.cpp 
> afd7ff4f2b50cb20cc2c8865b655ad1f8eb0c8b7 
> 
> Diff: https://reviews.apache.org/r/35157/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Reply via email to