Thanks for the review Vijay. I will fix these loose-ends before merge. Agree with site info being part of 'GlobalLoadBalancerResponse' object. I will incorporate it.
On 20/03/13 9:18 AM, "Vijay Venkatachalam" <vijay.venkatacha...@citrix.com> wrote: > >----------------------------------------------------------- >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/CreateGlobal >LoadBalancerRuleCmd.java ><https://reviews.apache.org/r/10021/#comment38263> > > Typo in description > > > >api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/ListGlobalLo >adBalancerRuleCmd.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/RemoveFromGl >obalLoadBalancerRuleCmd.java ><https://reviews.apache.org/r/10021/#comment38265> > > typo, gloabal; assigned == removed > > > >api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/UpdateGlobal >LoadBalancerRuleCmd.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/AddNetscaler >LoadBalancerCmd.java ><https://reviews.apache.org/r/10021/#comment38268> > > Typo. "Private IP" > > > >plugins/network-elements/netscaler/src/com/cloud/network/element/Netscaler >Element.java ><https://reviews.apache.org/r/10021/#comment38269> > > IMPORTANT: Isnt it "!=" null? > > > >plugins/network-elements/netscaler/src/com/cloud/network/resource/Netscale >rResource.java ><https://reviews.apache.org/r/10021/#comment38270> > > Create in remove path? > > > >plugins/network-elements/netscaler/src/com/cloud/network/resource/Netscale >rResource.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/AssignToGlo >>balLoadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/CreateGloba >>lLoadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/DeleteGloba >>lLoadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/ListGlobalL >>oadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/RemoveFromG >>lobalLoadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/command/user/region/ha/gslb/UpdateGloba >>lLoadBalancerRuleCmd.java PRE-CREATION >> >>api/src/org/apache/cloudstack/api/response/GlobalLoadBalancerResponse.jav >>a 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/F5ExternalLoadB >>alancerElement.java 3e75c3f >> >>plugins/network-elements/netscaler/src/com/cloud/api/commands/AddNetscale >>rLoadBalancerCmd.java 80c8cb9 >> >>plugins/network-elements/netscaler/src/com/cloud/network/element/Netscale >>rElement.java a90440c >> >>plugins/network-elements/netscaler/src/com/cloud/network/resource/Netscal >>erResource.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.ja >>va PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapD >>ao.java PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapD >>aoImpl.java PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerLbRuleMapV >>O.java PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerRuleDao.ja >>va PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancerRuleVO.jav >>a PRE-CREATION >> >>server/src/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesServ >>iceImpl.java PRE-CREATION >> server/src/org/apache/cloudstack/region/gslb/GslbServiceProvider.java >>PRE-CREATION >> >>server/test/org/apache/cloudstack/region/gslb/GlobalLoadBalancingRulesSer >>viceImplTest.java PRE-CREATION >> setup/db/db/schema-410to420.sql 4e39a71 >> >> Diff: https://reviews.apache.org/r/10021/diff/ >> >> >> Testing >> ------- >> >> >> Thanks, >> >> Murali Reddy >> >> > >
