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