cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Vijay Venkatachalam" <vijay.venkatacha...@citrix.com>
Subject Re: Review Request: review request for merging GSLB feature into master
Date Wed, 20 Mar 2013 03:48:28 GMT

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


Wanted to go through this feature but never got time during the FS, so went through code :-).
Overall Comment: Pristine. Caught some nitpicks. Comments which are important are marked "IMPORTANT".


Overall Comment:
1. It will be good to have the zone names (site info) be listed as part of the ListGSLB
2. The imports have been condensed into "*" in many files


api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/CreateGlobalLoadBalancerRuleCmd.java
<https://reviews.apache.org/r/10021/#comment38263>

    Typo in description



api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/ListGlobalLoadBalancerRuleCmd.java
<https://reviews.apache.org/r/10021/#comment38264>

    IMPORTANT: List will not work now is it?



api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/RemoveFromGlobalLoadBalancerRuleCmd.java
<https://reviews.apache.org/r/10021/#comment38265>

    typo, gloabal; assigned == removed



api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/UpdateGlobalLoadBalancerRuleCmd.java
<https://reviews.apache.org/r/10021/#comment38266>

    IMPORTANT: Typo



api/src/org/apache/cloudstack/api/response/GlobalLoadBalancerResponse.java
<https://reviews.apache.org/r/10021/#comment38267>

    Project is not part of the input, but part of the output. Should the response be even
a controlled entity?



plugins/network-elements/netscaler/src/com/cloud/api/commands/AddNetscalerLoadBalancerCmd.java
<https://reviews.apache.org/r/10021/#comment38268>

    Typo. "Private IP"



plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java
<https://reviews.apache.org/r/10021/#comment38269>

    IMPORTANT: Isnt it "!=" null?



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/10021/#comment38270>

    Create in remove path?



plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
<https://reviews.apache.org/r/10021/#comment38271>

    This is taken care already by the caller, where it checks if a siteobject already exists.
May be we can remove in the caller and use a name like applySite here?



server/src/com/cloud/api/ApiResponseHelper.java
<https://reviews.apache.org/r/10021/#comment38280>

    IMPORTANT: Account, Domain...not set!


- Vijay Venkatachalam


On March 20, 2013, 1:43 a.m., Murali Reddy wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/10021/
> -----------------------------------------------------------
> 
> (Updated March 20, 2013, 1:43 a.m.)
> 
> 
> Review request for cloudstack and Murali Reddy.
> 
> 
> Description
> -------
> 
> Merge request for GSLB feature branch. 
> 
> 
> Diffs
> -----
> 
>   api/src/com/cloud/agent/api/routing/GlobalLoadBalancerConfigAnswer.java PRE-CREATION

>   api/src/com/cloud/agent/api/routing/GlobalLoadBalancerConfigCommand.java PRE-CREATION

>   api/src/com/cloud/agent/api/routing/SiteLoadBalancerConfig.java PRE-CREATION 
>   api/src/com/cloud/async/AsyncJob.java 034c853 
>   api/src/com/cloud/event/EventTypes.java f38865c 
>   api/src/com/cloud/region/ha/GlobalLoadBalancerRule.java PRE-CREATION 
>   api/src/com/cloud/region/ha/GlobalLoadBalancingRulesService.java PRE-CREATION 
>   api/src/org/apache/cloudstack/api/ApiConstants.java f4c6c52 
>   api/src/org/apache/cloudstack/api/ResponseGenerator.java 628a185 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/AssignToGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/CreateGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/DeleteGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/ListGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/RemoveFromGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/UpdateGlobalLoadBalancerRuleCmd.java
PRE-CREATION 
>   api/src/org/apache/cloudstack/api/response/GlobalLoadBalancerResponse.java PRE-CREATION

>   api/src/org/apache/cloudstack/region/Region.java f8926ee 
>   client/tomcatconf/commands.properties.in 382573b 
>   plugins/network-elements/f5/src/com/cloud/network/element/F5ExternalLoadBalancerElement.java
3e75c3f 
>   plugins/network-elements/netscaler/src/com/cloud/api/commands/AddNetscalerLoadBalancerCmd.java
80c8cb9 
>   plugins/network-elements/netscaler/src/com/cloud/network/element/NetscalerElement.java
a90440c 
>   plugins/network-elements/netscaler/src/com/cloud/network/resource/NetscalerResource.java
4eb0ce2 
>   server/src/com/cloud/api/ApiResponseHelper.java 663139d 
>   server/src/com/cloud/api/ApiServer.java 0439c6e 
>   server/src/com/cloud/configuration/Config.java 9db7dbd 
>   server/src/com/cloud/network/ExternalLoadBalancerDeviceManager.java dee3ca9 
>   server/src/com/cloud/network/ExternalLoadBalancerDeviceManagerImpl.java 049099d 
>   server/src/com/cloud/network/dao/ExternalLoadBalancerDeviceDao.java 1bd2107 
>   server/src/com/cloud/network/dao/ExternalLoadBalancerDeviceDaoImpl.java e559fad 
>   server/src/com/cloud/network/dao/ExternalLoadBalancerDeviceVO.java cd9dffd 
>   server/src/com/cloud/network/lb/LoadBalancingRulesManagerImpl.java a06cbc5 
>   server/src/com/cloud/server/ManagementServerImpl.java 1f1f12e 
>   server/src/org/apache/cloudstack/region/RegionServiceProvider.java PRE-CREATION 
>   server/src/org/apache/cloudstack/region/RegionVO.java 907c11d 
>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerDaoImpl.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapDao.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapDaoImpl.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapVO.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerRuleDao.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerRuleVO.java PRE-CREATION

>   server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImpl.java
PRE-CREATION 
>   server/src/org/apache/cloudstack/region/gslb/GslbServiceProvider.java PRE-CREATION

>   server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServiceImplTest.java
PRE-CREATION 
>   setup/db/db/schema-410to420.sql 4e39a71 
> 
> Diff: https://reviews.apache.org/r/10021/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Murali Reddy
> 
>


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