You are saying it must be null and not empty?
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
