----------------------------------------------------------- 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 > >
