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. > > Rohit Yadav wrote: > 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.
I will, good idea! - Ilia ----------------------------------------------------------- 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 > >
