Can you address 3-6 ? On 8/16/12 7:52 AM, "Ram Ganesh" <ram.gan...@citrix.com> wrote:
>Changing the subject line to reflect feature being discussed.... > >Chiradeep, > >> -----Original Message----- >> From: Chiradeep Vittal [mailto:chiradeep.vit...@citrix.com] >> Sent: 15 August 2012 07:32 >> To: CloudStack DeveloperList >> Cc: Pranav Saxena; David Nalley; Vijay Venkatachalam; Alena >> Prokharchyk; Deepak Garg >> Subject: Re: where features are developed was: Review Request: Merge >> Kelven's VPC code for Vmware into asf vpc branch >> >> I have a few comments: >> 1. The table autoscale_vmprofiles has account_id as well as user_id. >> Only >> account_id should be sufficient? > > User_id is critical here. This is the handle used by the NetworkElement, >NetScaler, to issue DeployVM and DestroyVM API calls to CloudStack >management server. Column name in autoscale_vmprofiles db table is >autoscale_user_id. > > >> 2. The table 'counter' and 'condition' should be renamed so that it is >> clear that it is autoscaling related. So also the corresponding Java >> classes, APIs and interfaces > > The idea is to look at Counter and Condition as generic entities which >can be used in various use cases and autoscale is one of such use case. >Tomorrow once we have a robust messaging framework(per Murali's proposal) >in CloudStack one could attach outbound actions like email send, SNMP >event forward to a third-party SNMP manager say once a condition is >evaluated to true . Hence we do not want to restrict these entities to >AutoScale. Please let us know your thoughts. > >> 3. Any indexes at all on these new tables? >> 4. autoscale_vmprofiles (and java class) should not have reference to >> snmp. We discussed why before, and alternatives to this design on this >> mailing list. >> 5. A lot of the changes to existing files are whitespace changes that >> are >> unrelated to the logic. Please avoid this. >> 6. Why is a Counter not a ControlledEntity but Condition is? >> 7. Not sure why the LoadBalancer* classes have been touched. Everything >> should be isolated to Autoscale? Can you explain why you had to make >> API >> changes to the LoadBalancingRulesManager? > > It is a Loadbalancer rule which is enabled/configured for AutoScale. The >changes in Loadbalancer* classes are only to bind the AutoScale feature >to an Loadbalancer rule. Rest all entities such as Counter, Conditions, >AutoScaleProfile etc reside outside Loadbalancer* classes. > >> 8. The License header is not consistent (some with leading spaces, >> others >> without) >> 9. Not all files have the Apache license. >> > >Thanks, >Ram >
