On 11-Mar-2014, at 7:24 PM, Daan Hoogland <daan.hoogl...@gmail.com> wrote:
> You are saying it must be null and not empty? [Koushik] Yes. > > > On Tue, Mar 11, 2014 at 1:41 PM, Koushik Das <koushik....@citrix.com> wrote: > >> This is an automatically generated e-mail. To reply, visit: >> https://reviews.apache.org/r/19022/ >> >> On March 11th, 2014, 7:41 a.m. UTC, *daan Hoogland* wrote: >> >> >> server/src/com/cloud/api/query/QueryManagerImpl.java<https://reviews.apache.org/r/19022/diff/1/?file=516059#file516059line731> >> (Diff >> revision 1) >> >> public class QueryManagerImpl extends ManagerBase implements QueryService { >> >> 731 >> >> List<Long> ids = null; >> >> Why not be lenient and consider the id as part of ids? >> You are being strict on something that isn't hurtful but an empty ids when >> no id is given is not checked! >> >> On March 11th, 2014, 8:34 a.m. UTC, *Koushik Das* wrote: >> >> I haven't removed 'id' due to back-compat. As I have mentioned 'id' and >> 'ids' are mutually exclusive. Also it is a valid scenario to not pass any of >> id or ids. >> >> On March 11th, 2014, 10:56 a.m. UTC, *daan Hoogland* wrote: >> >> ok, I don't see why id and ids should be mutually exclusive, but i don't >> mind either. If passing no id at all is a valid scenario then it is allright >> to not check but I would still initialize the list and addAll to it. Not an >> issue thought! >> >> Thanks for the comment Daan. >> >>>>> I would still initialize the list and addAll to it >> I don't quite follow what do you mean by addAll to the list. The sql query >> that gets generated looks like "where id in (1, 2, 3)" if the list is >> non-null. If there is no id specified then the list is null and is not >> considered while building the query. >> >> >> - Koushik >> >> On March 11th, 2014, 6:29 a.m. UTC, Koushik Das wrote: >> Review request for cloudstack, daan Hoogland and Min Chen. >> By Koushik Das. >> >> *Updated March 11, 2014, 6:29 a.m.* >> *Bugs: * >> CLOUDSTACK-6052<https://issues.apache.org/jira/browse/CLOUDSTACK-6052> >> *Repository: * cloudstack-git >> Description >> >> Currently list VM can only be called using a single VM ID. So if there is a >> need to query a set of VMs using ID then either multiple list VM calls need >> to be made or all VMs needs to be fetched and then do a client side >> filtering. Both approaches are sub-optimal - the former results in multiple >> queries to database and the latter will be an overkill if you need a small >> subset from a very large number of VMs. >> >> The proposal is to have an additional parameter to specify a list of VM IDs >> for which the data needs to be fetched. Using this the required VMs can be >> queried in an efficient manner. With the new parameter the syntax would look >> like >> http://localhost:8096/api?command=listVirtualMachines&listAll=true&ids=eddac053-9b12-4d2e-acb7-233de2e98112,009966fc-4d7b-4f84-8609-254979ba0134 >> >> The new 'ids' parameter will be mutually exclusive with the existing 'id' >> parameter. >> >> Testing >> >> Added integration test, also verified manually. >> >> Diffs >> >> - api/src/org/apache/cloudstack/api/command/user/vm/ListVMsCmd.java >> (1a564f6) >> - server/src/com/cloud/api/query/QueryManagerImpl.java (4200799) >> - test/integration/smoke/test_deploy_vm.py (425aeb7) >> >> View Diff <https://reviews.apache.org/r/19022/diff/> >> > > > > -- > Daan
