On Sept. 9, 2014, 8:55 a.m., Ilia Shakitko wrote: > > By using variable type to CommandType.STRING, we run the risk of blowing up > > inner layers if they are UUIDs. If there are other places where STRING is > > used instead of UUID, they must be fixed too. By using the commandtype > > UUID, the apidocs will get this information right and also clients using > > listApis such as cloudmonkey's sync and it sfuzzy parameter completer. You > > simply need to change the type to CommandType.UUID, the variable is still a > > String. You may read in detail why it should be UUID, > > https://cwiki.apache.org/confluence/display/CLOUDSTACK/CloudStack+API+refactoring > > Ilia Shakitko wrote: > I tried. It throws an exception because it tries to map it with an > Entity. But the entityType is not provided. I just tried to change > CommandType.STRING => CommandType.UUID and it didn't work > > Rohit Yadav wrote: > yes, in the parameter annotation can you try adding entityType = > UsageTypeResponse.class and see if this works? > > Ilia Shakitko wrote: > I can, but did you try that way? entityType should be the > XxxxResponce.class where it will search for an UUID<->ID transformation. > Logically speaking UsageTypeResponse shouldn't be there, what will it use > for? The parameter is UsageId and we are forcing CS to consider it as > UsageType... > > Rohit Yadav wrote: > I did not check if UsageTypeResponse had an ID field for doing the > transformation; You're right, so you may add a new response class or modify > the usage type response class so that UUID<-> ID transformation will happen > fine. We should avoid shortcuts if possible. > > Ilia Shakitko wrote: > If possible. It will be quite expensive and complex to build a map in new > response class. Usage ID can be related to many of resources, and usage type > won't be available in that new created Response.class, so how do you suggest > map the UUID to one of the PROPER entities in the Response.class? In the > UsageService implementation I can understand, I can pick the usageType (from > the command) and operate it. > > Rohit Yadav wrote: > Alright, how about you fix the syntax on @Parameter and I'll take it > further and fix the ID<->UUID thing afterwards. > > Ilia Shakitko wrote: > Collaboration is the everything we have! Could you include me in review > or just notify in the JIRA ticket then? I'd like to participate in further > refactoring. Maybe I'll do that for the other places where we have UUID > accepted in comand as string.
Yes. At present I understand changing it UUID would involve changes at some places and since it's is not consistent everywhere so this should be done as part of refactoring so but at the same time I want to include your patch on master soon. Why don't you open a JIRA issue to fix the UUID stuff here and make it consistent everywhere else. Tag me to that ticket as well so I can collaborate with you. - Rohit ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/25435/#review52693 ----------------------------------------------------------- On Sept. 9, 2014, 1:44 p.m., Ilia Shakitko wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/25435/ > ----------------------------------------------------------- > > (Updated Sept. 9, 2014, 1:44 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 > >
