mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Neil Conway" <neil.con...@gmail.com>
Subject Re: Review Request 41225: Added test cases for implicit roles.
Date Fri, 18 Dec 2015 21:28:01 GMT


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 107
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line107>
> >
> >     Is this necessary? The defaultAgentResourcesString is "cpus:2;mem:1024;disk:1024;ports:[31000-32000]"
> >     Overriding that means that Mesos will auto-detect cpus/mem

Personally, I think it is better to be more explicit: when I read the test case code, I can
see that resource specification for the slave, rather than relying on `defaultAgentResourcesString`
which is defined someplace else.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 120
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line120>
> >
> >     s/by default/the default/

```
$ ag "be filtered for 5 seconds (by default)" src/tests | wc -l
      18
```

i.e., this text occurs in many tests. I think we should use the same text in each place. I'll
open a separate review request to change them all.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 385
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line385>
> >
> >     Seems we don't have any tests with weights yet. Congrats on writing the first!

Seems like we need tests for allocation behavior in the presence of weights, at a bare minimum.
I filed a JIRA: https://issues.apache.org/jira/browse/MESOS-4200


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, line 47
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line47>
> >
> >     Why is this an instance member method of RoleTest? Seems idempotent and stateless
enough that it could be a static method, or even a freestanding helper function. If this is
only called once, why is it even a function at all?
> >     
> >     Oh.. it's copied from `MasterQuotaTest::createRequestBody()`.
> >     
> >     Either find a way to actually merge the two implementations, or just inline
this into the one test that uses it.

Lots of tests use stateless methods in their `MesosTest` sub-class to do things like create
HTTP request bodies. Not sure why, but I thought I'd be consistent with the existing convention.

I just got rid of the function.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 165-166
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line165>
> >
> >     Can you make these "volumeId1" and "/container/path" so it's clearer what they're
supposed to be?

"id1" and "path1" matches the other call sites of `createPersistentVolume()`, so I'd vote
for consistency. If you'd like me to change all the call-sites in a separate review, let me
know.


> On Dec. 18, 2015, 7:37 a.m., Adam B wrote:
> > src/tests/role_tests.cpp, lines 251-259
> > <https://reviews.apache.org/r/41225/diff/7/?file=1170532#file1170532line251>
> >
> >     Why bother to send back the ack? Or even send the status back at all? As soon
as you have a MockExecutor that receives a launchTask, you've verified that "when using implicit
roles, a static reservation can be used to launch a task"

After you do `driver.acceptOffers({offer.id()}, {LAUNCH({taskInfo})}, filters);`, how do you
verify that the master has received the launch task attempt before the test case shuts down?
It seems to me you need some form of synchronization (and indeed, removing the code that waits
for the status ack causes the test to fail).


- Neil


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


On Dec. 18, 2015, 8:01 p.m., Neil Conway wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/41225/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2015, 8:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, and Yongqiao Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added test cases for implicit roles.
> 
> 
> Diffs
> -----
> 
>   src/tests/role_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/41225/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Neil Conway
> 
>


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