cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ilia Shakitko" <i.shaki...@tech.leaseweb.com>
Subject Re: Review Request 25435: [CLOUDSTACK-7159] Add "usageid" parameter to the "listUsageRecords" API call.
Date Mon, 08 Sep 2014 15:33:03 GMT


> On Sept. 8, 2014, 1:52 p.m., Rohit Yadav wrote:
> > api/src/org/apache/cloudstack/api/command/admin/usage/GetUsageRecordsCmd.java, lines
73-74
> > <https://reviews.apache.org/r/25435/diff/1/?file=682558#file682558line73>
> >
> >     Does this cause any issue? I would avoid this though there is no much different,
long will occupy some more bytes than Int, but it would require us to note change in the API.
> 
> Ilia Shakitko wrote:
>     1) UsageTypes are Inegers
>     2) usageRecord.getUsageType() is returning an Integer
>     3) public void setUsageType(Integer usageType) { ... }
>     
>     And If I'll change it to Long back, I'll have to either transform usageType (here:
"switch (usageType)") to an Integer (to support switch, as of usageTypes are Integers) or
use if-else statements, what I don't like in this particular place.
>     
>     Convinced? :)
> 
> Rohit Yadav wrote:
>     I'm just trying to help you with the review, my opinion is that one should avoid
changing API signature so either let Java auto-unbox/cast this from Long to Integer for you,
or fix usage types to Long.
> 
> Ilia Shakitko wrote:
>     If I will change UsageTypes to Long it will be more impact. But anyway, if it is
required I'll add a cast to Integer, which I don't like of course.
> 
> Ilia Shakitko wrote:
>     It is an integer everywhere, but in command parameters. I just wanted to make it
more clear, fixnig the type. But I can leave it as is.
> 
> Rohit Yadav wrote:
>     Thanks for checking. To make this consistent, should we make all the APIs usageTypes
params to Int? Your patch is fine in that case, just fix the uuid and other syntax things.

> should we make all the APIs usageTypes params to Int

I don't think I get you correctly. UsageTypes.java had all the usage types Integers. 

Meanwhile I almost finished the second revision of patch with "Long" type used. Then I just
do intVal().


> On Sept. 8, 2014, 1:52 p.m., Rohit Yadav wrote:
> > api/src/org/apache/cloudstack/api/command/admin/usage/GetUsageRecordsCmd.java, lines
76-77
> > <https://reviews.apache.org/r/25435/diff/1/?file=682558#file682558line76>
> >
> >     If this is going to be UUID, please use the CommandType.UUID?

In this case I'll be forced to use entityType = XXXXResponse.class in the @parameter. But
here I can't explicitly set it because it's dependant on the usageType is provided. And then
I use SWITCH block to determine where to check on the usageid existance. That's why I used
String for that parameter. I've also seen few places where UUID parameter is being accepted
as String. Probably, to achieve same kind of goal.


- Ilia


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


On Sept. 8, 2014, 1:45 p.m., Ilia Shakitko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25435/
> -----------------------------------------------------------
> 
> (Updated Sept. 8, 2014, 1:45 p.m.)
> 
> 
> Review request for cloudstack, Alena Prokharchyk, Kishan Kavala, and Sheng Yang.
> 
> 
> Bugs: CLOUDSTACK-7159
>     https://issues.apache.org/jira/browse/CLOUDSTACK-7159
> 
> 
> Repository: cloudstack-git
> 
> 
> Description
> -------
> 
> Working with Usage server and usage records very often I need to get only records for
that particular usage ID. For example when filtering out network_bytes_received/sent with
big amount of data it's not very fast to process hundreds of objects looking for the only
one you need.
> It would be useful to have an ability to filter out usage records only for specific resource
ID.
> 
> This parch brings that to the API.
> 
> 
> Diffs
> -----
> 
>   api/src/org/apache/cloudstack/api/ApiConstants.java 6baa95c 
>   api/src/org/apache/cloudstack/api/command/admin/usage/GetUsageRecordsCmd.java 21a7e4a

>   server/src/com/cloud/usage/UsageServiceImpl.java d1f62aa 
> 
> Diff: https://reviews.apache.org/r/25435/diff/
> 
> 
> Testing
> -------
> 
> Tested cases:
> - usageid is specified w/o "type": an exception is thrown (correct)
> - provided usageid is not exists: an empty response is returned (since no records were
found, correct)
> - no usageid specified: work as is
> - an existing usageid specified (with type, for example type=4 or type=5): only records
for that usage type is returned
> 
> 
> Thanks,
> 
> Ilia Shakitko
> 
>


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