mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bartek Plotka" <bwplo...@gmail.com>
Subject Re: Review Request 35157: Added unit tests for fetching ResourceUsage in both QoS Controller and Resource Estimator .
Date Tue, 09 Jun 2015 00:22:22 GMT


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


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