cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alena Prokharchyk" <alena.prokharc...@citrix.com>
Subject Re: Review Request: CS-16104: Improve restart network behaviour for basic network
Date Fri, 14 Sep 2012 23:43:51 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6781/#review11575
-----------------------------------------------------------


1)                long dcId = dest.getDataCenter().getId();
				List<HostPodVO> pods = _podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User,
VirtualMachine.State.Running);
                pods.addAll(_podDao.listByDataCenterIdVMTypeAndState(dcId, VirtualMachine.Type.User,
VirtualMachine.State.Starting));


                Instead of calling method 2 times, you can add State... to the istByDataCenterIdVMTypeAndState
method and pass both states - Starting and Running - separated by coma.

                Please return list of PodIds in the response; we don't need the objects. The
dao method has to be placed 


2) The above applies to listUserVms as well:

List<DomainRouterVO> virtualRouters = _routerDao.listByPodIdAndState(podId, VirtualMachine.State.Running);
virtualRouters.addAll(_routerDao.listByPodIdAndState(podId, VirtualMachine.State.Starting));



Call one dao method, and only get the count().



3) Move this method to vmInstanceDaoImpl - it doesn't make any sense to keep it in podDaoImpl
as all we have to return - podIds, and the original search builder is done against vmInstanceDao.
And vm_instance table has pod_id/data_center_id fields, there is no reason to make a joins
with host_pod_ref.I


Also pass zoneId, vmState(s) - make it accept multiple state by defining State... in the method
signature, vmType parameters to the call:

    @Override
    public List<HostPodVO> listByDataCenterIdVMTypeAndState(long id, VirtualMachine.Type
type, VirtualMachine.State state) {
        final VMInstanceDaoImpl _vmDao = ComponentLocator.inject(VMInstanceDaoImpl.class);
        SearchBuilder<VMInstanceVO> vmInstanceSearch = _vmDao.createSearchBuilder();
        vmInstanceSearch.and("type", vmInstanceSearch.entity().getType(), SearchCriteria.Op.EQ);
        vmInstanceSearch.and("state", vmInstanceSearch.entity().getState(), SearchCriteria.Op.EQ);

        SearchBuilder<HostPodVO> podIdSearch = createSearchBuilder();
        podIdSearch.and("dc", podIdSearch.entity().getDataCenterId(), SearchCriteria.Op.EQ);
        podIdSearch.select(null, SearchCriteria.Func.DISTINCT, podIdSearch.entity().getId());
        podIdSearch.join("vmInstanceSearch", vmInstanceSearch, podIdSearch.entity().getId(),
                vmInstanceSearch.entity().getPodIdToDeployIn(), JoinBuilder.JoinType.INNER);
        podIdSearch.done();

        SearchCriteria<HostPodVO> sc = podIdSearch.create();
        sc.setParameters("dc", id);
        sc.setJoinParameters("vmInstanceSearch", "type", type);
        sc.setJoinParameters("vmInstanceSearch", "state", state);
        return listBy(sc);
    }




4) Remove this method from domainRouterDaoImpl:

@Override
+    public List<DomainRouterVO> listByPodId(Long podId) {
+        SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
+        sc.setParameters("podId", podId);
+        return listBy(sc);
+    }



This method can cover both cases - search by pod and search by podId - if State is changed
to State... in the method signature:


@Override
+    public List<DomainRouterVO> listByPodIdAndState(Long podId, State state) {
+        SearchCriteria<DomainRouterVO> sc = AllFieldsSearch.create();
+        sc.setParameters("podId", podId);
+        sc.setParameters("state", state);
+        return listBy(sc);
+    }

State... can accept either none, or one, or multiple values for state.

- Alena Prokharchyk


On Sept. 12, 2012, 11:32 a.m., Rohit Yadav wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/6781/
> -----------------------------------------------------------
> 
> (Updated Sept. 12, 2012, 11:32 a.m.)
> 
> 
> Review request for cloudstack, Abhinandan Prateek, Alena Prokharchyk, and Chiradeep Vittal.
> 
> 
> Description
> -------
> 
> Patch as per discussion on ML and http://bugs.cloudstack.org/browse/CS-16104
> CLOUDSTACK-70 on issues.apache.org
> 
> Download original patch: http://patchbin.baagi.org/p?id=yfawv5
> git am <patch> # to apply retaining commit message
> 
> 
> This addresses bug CS-16104.
> 
> 
> Diffs
> -----
> 
>   server/src/com/cloud/dc/dao/HostPodDao.java f031ac9 
>   server/src/com/cloud/dc/dao/HostPodDaoImpl.java bec8c51 
>   server/src/com/cloud/network/NetworkManagerImpl.java 292a259 
>   server/src/com/cloud/network/router/VirtualNetworkApplianceManagerImpl.java 6618fdf

>   server/src/com/cloud/vm/dao/DomainRouterDao.java 01e3258 
>   server/src/com/cloud/vm/dao/DomainRouterDaoImpl.java 175d3f2 
>   server/src/com/cloud/vm/dao/VMInstanceDao.java 2cf3d75 
>   server/src/com/cloud/vm/dao/VMInstanceDaoImpl.java 571b5d1 
>   ui/scripts/network.js d0f65c4 
> 
> Diff: https://reviews.apache.org/r/6781/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Rohit Yadav
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message