aurora-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Suman Karumuri" <ma...@apache.org>
Subject Re: Review Request 19509: Now consuming nonProd consumption from the new getQuota API
Date Mon, 24 Mar 2014 20:44:02 GMT


> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 47
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line47>
> >
> >     > Currently, the mocked calls can't detect renamed and missing thrift structs.
> >     
> >     This is one unfortunate aspect of the thrift API, you can set arbitrary attributes.
 However, you can harden the tests a bit more if you use the constructors.  These would trip
up on a refactor.  I find the python repl in pants helpful to see the APIs (see below).  I
suggest taking the route of using constructors everywhere, probably introducing a helper method
or two to cut down on the verbosity.
> >     
> >     $ ./pants py src/main/thrift/org/apache/aurora/gen:py-thrift-packaged
> >     Python 2.6.8 (unknown, Aug 25 2013, 00:04:29)
> >     [GCC 4.2.1 Compatible Apple LLVM 5.0 (clang-500.0.68)] on darwin
> >     Type "help", "copyright", "credits" or "license" for more information.
> >     (InteractiveConsole)
> >     >>> from gen.apache.aurora.ttypes import *
> >     >>> help(GetQuotaResult)
> >     
> >     Help on class GetQuotaResult in module gen.apache.aurora.ttypes:
> >     
> >     class GetQuotaResult(__builtin__.object)
> >      |  Attributes:
> >      |   - quota
> >      |   - prodConsumption
> >      |   - nonProdConsumption
> >      |
> >      |  Methods defined here:
> >      |
> >      |  __eq__(self, other)
> >      |
> >      |  __init__(self, quota=None, prodConsumption=None, nonProdConsumption=None)

Agreed that a constructing an object using the constructor is the best way here. Updated the
code. 


> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 77
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line77>
> >
> >     Be consistent about single and double quotes, here and below.  That said, this
might be easier to grok with a multiline string:
> >     
> >     
> >     '''Allocated:
> >     CPU: 5
> >     RAM: 20.000000 GB
> >     Disk: 40.000000 GB'''
> >     
> >     While you're at it, can you add formatting to the float values?  %g as opposed
to %s is probably appropriate.

We are already using %f here which per the docs is a human readable version of %g. So, leaving
it as is. Updated quoting to be consistent.

Looked into using a multi-line string, but the text doesn't format that well because of additional
spaces in the output. Since this is also the output users see and is a stylistic change, refraining
from making any output format changes in this diff.


> On March 21, 2014, 11:53 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_quota.py, line 91
> > <https://reviews.apache.org/r/19509/diff/2/?file=532010#file532010line91>
> >
> >     Thanks for this coverage!  Do you know if the JSON stuff will be brittle w.r.t.
field ordering?  That's bitten us in the past.  In these cases, it's probably best to deserialize
the json coming from the SUT and check for object (dict) equality.

Good one. Updated the code, to turn json into dict and comparing the dicts.


- Suman


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


On March 21, 2014, 10:14 p.m., Suman Karumuri wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19509/
> -----------------------------------------------------------
> 
> (Updated March 21, 2014, 10:14 p.m.)
> 
> 
> Review request for Aurora, Kevin Sweeney, Mark Chu-Carroll, and Brian Wickman.
> 
> 
> Bugs: AURORA-246
>     https://issues.apache.org/jira/browse/AURORA-246
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Updated the client to consume the new getQuota API which contains nonProdConsumption(resources
consumed by non-prod tasks) information also. 
> 
> Currently, the mocked calls can't detect renamed and missing thrift structs. Added tests
to look for expected fields in GetQuotaResult and  ResourceAggregate structs. Refactored tests
in test_quota to remove duplicated code and added a missing test.
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/client/cli/quota.py d06f21a80575058aefa3dffc72b365805d7a5ce2

>   src/main/python/apache/aurora/client/commands/core.py 9977c725528086d3e8cf58de294adee542570411

>   src/test/python/apache/aurora/client/cli/test_quota.py 44afd74aa5b11296951f45fe7edca8cb58b0ec18

>   src/test/sh/org/apache/aurora/e2e/test_end_to_end.sh 27b745ea31189a0ea0731619eb4c06f802aa04b9

> 
> Diff: https://reviews.apache.org/r/19509/diff/
> 
> 
> Testing
> -------
> 
> ./pants src/test/python:all -vxs 
> 
> Works for all quota related test. src.test.python.apache.thermos.bin.test_thermos fails
on my laptop because of build issues. Sending out a code review since it is an un-related
issue. Will look into this tomorrow, if it's a blocker.
> 
> Found all the code to update by running the following query.
> 
> ?  git:(mansu/AURORA-246_getQuotaAPI) ag getQuotaResult -G '.py'                    
                                                                   ~/workspace/incubator-aurora
> src/main/python/apache/aurora/client/api/quota_check.py
> 90:    allocated = CapacityRequest(resp.result.getQuotaResult.quota)
> 91:    consumed = CapacityRequest(resp.result.getQuotaResult.prodConsumption)
> 
> src/main/python/apache/aurora/client/cli/quota.py
> 65:      return serialize(quota_resp.result.getQuotaResult,
> 69:      result += get_quota_str(quota_resp.result.getQuotaResult.quota)
> 70:      if quota_resp.result.getQuotaResult.prodConsumption:
> 72:        result += get_quota_str(quota_resp.result.getQuotaResult.prodConsumption)
> 73:      if quota_resp.result.getQuotaResult.nonProdConsumption:
> 75:        result += get_quota_str(quota_resp.result.getQuotaResult.nonProdConsumption)
> 
> src/main/python/apache/aurora/client/commands/admin.py
> 199:  quota = resp.result.getQuotaResult.quota
> 
> src/main/python/apache/aurora/client/commands/core.py
> 632:  print_quota(resp.result.getQuotaResult.quota, 'Total allocated quota', role)
> 634:  if resp.result.getQuotaResult.prodConsumption:
> 635:    print_quota(resp.result.getQuotaResult.prodConsumption,
> 639:  if resp.result.getQuotaResult.nonProdConsumption:
> 640:    print_quota(resp.result.getQuotaResult.nonProdConsumption,
> 
> src/test/python/apache/aurora/client/api/test_quota_check.py
> 49:        getQuotaResult=GetQuotaResult(
> 
> src/test/python/apache/aurora/client/cli/test_quota.py
> 35:    response.result.getQuotaResult = GetQuotaResult()
> 36:    response.result.getQuotaResult.quota = ResourceAggregate()
> 37:    response.result.getQuotaResult.quota.numCpus = 5
> 38:    response.result.getQuotaResult.quota.ramMb = 20480
> 39:    response.result.getQuotaResult.quota.diskMb = 40960
> 40:    response.result.getQuotaResult.consumed = None
> 47:    response.result.getQuotaResult = GetQuotaResult()
> 48:    response.result.getQuotaResult.quota = ResourceAggregate()
> 49:    response.result.getQuotaResult.quota.numCpus = 5
> 50:    response.result.getQuotaResult.quota.ramMb = 20480
> 51:    response.result.getQuotaResult.quota.diskMb = 40960
> 52:    response.result.getQuotaResult.prodConsumption = ResourceAggregate()
> 53:    response.result.getQuotaResult.prodConsumption.numCpus = 1
> 54:    response.result.getQuotaResult.prodConsumption.ramMb = 1024
> 55:    response.result.getQuotaResult.prodConsumption.diskMb = 2048
> 56:    response.result.getQuotaResult.nonProdConsumption = ResourceAggregate()
> 57:    response.result.getQuotaResult.nonProdConsumption.numCpus = 1
> 58:    response.result.getQuotaResult.nonProdConsumption.ramMb = 1024
> 59:    response.result.getQuotaResult.nonProdConsumption.diskMb = 2048
> ?  git:(mansu/AURORA-246_getQuotaAPI)                                               
                                                                   ~/workspace/incubator-aurora
> 
> 
> Thanks,
> 
> Suman Karumuri
> 
>


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