[
https://issues.apache.org/jira/browse/CLOUDSTACK-8750?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14721241#comment-14721241
]
ASF subversion and git services commented on CLOUDSTACK-8750:
-------------------------------------------------------------
Commit cd7218e241a8ac93df7a73f938320487aa526de6 in cloudstack's branch
refs/heads/master from [~dahn]
[ https://git-wip-us.apache.org/repos/asf?p=cloudstack.git;h=cd7218e ]
Merge pull request #714 from rafaelweingartner/master-lrg-cs-hackday-003
Changed variable s_logger to non-static and fixed its name in
com.cloud.utils.component.ComponentLifecycleBase and its subclassesHi guys,
We have noticed that every single class that is a subclass of
ComponentLifecycleBase instantiate their on logger manually and uses a
nonstandard name. We fixed that by changing the variable in
ComponentLifecycleBase to protected and non-static and instantiated it using
the method getClass from Object class. Therefore, we can reduce the code in a
few hundred lines and use a more intuitive name for the logger variable.
During that process we found a static method that used the s_logger variable in
classes:
com.cloud.network.element.VirtualRouterElement
org.apache.cloudstack.network.element.InternalLoadBalancerElement
To fix that we had to create a new class
com.cloud.network.element.HAProxyLBRule, instantiate it with @Componente and
inject into the aforementioned classes.
The class that we create is com.cloud.network.element.HAProxyLBRule and has the
following methods:
com.cloud.network.element.HAProxyLBRule.containsOnlyNumbers(String, String)
com.cloud.network.element.HAProxyLBRule.validateHAProxyLBRule(LoadBalancingRule)
Sadly we could not write test cases to it; hence we did not fully understand
those methods. However, if anyone out there understands it, we would appreciate
some code to be added to it.
As minor this change may seem; we believe that it enhances a little bit the ACS
code by using standard name to logger variable.
* pr/714:
Solved jira ticket: CLOUDSTACK-8750
Signed-off-by: Daan Hoogland <[email protected]>
> Change the variable s_logger to non-static and fixed its name in
> “com.cloud.utils.component.ComponentLifecycleBase” and its subclasses
> --------------------------------------------------------------------------------------------------------------------------------------
>
> Key: CLOUDSTACK-8750
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-8750
> Project: CloudStack
> Issue Type: Improvement
> Security Level: Public(Anyone can view this level - this is the
> default.)
> Components: Management Server
> Reporter: pedro henrique pereira martins
> Assignee: pedro henrique pereira martins
> Priority: Minor
> Fix For: Future
>
> Original Estimate: 48h
> Remaining Estimate: 48h
>
> We have noticed that every single class that is a subclass of
> “ComponentLifecycleBase” instantiate their on “logger” manually and uses a
> nonstandard name. We fixed that by changing the variable in
> “ComponentLifecycleBase” to protected and non-static and instantiated it
> using the method “getClass” from Object class. Therefore, we can reduce the
> code in a few hundred lines and use a more intuitive name for the logger
> variable.
> We do understand that “s_ something“ is a proper way to instantiate a static
> variable in ACS classes. However, in few of the subclasses of
> “com.cloud.utils.component.ComponentLifecycleBase” it was used names such as
> “LOGGER” (that is a proper name for static field as JAVA standards), log,
> status_logger and others.
> What we propose is to change the logger variable in
> “com.cloud.utils.component.ComponentLifecycleBase” to protected and remove
> its static and final declaration. Therefore we did the following in
> ComponentLifecycleBase:
> protected Logger logger = Logger.getLogger(getClass());
> This way, every single subclass of ComponentLifecycleBase, when
> instantiated would automatically have a logger instance for its proper class
> ready to be used.
> During that process we found a static method in class that used the
> “s_logger” variable in classes:
> com.cloud.network.element.VirtualRouterElement
> org.apache.cloudstack.network.element.InternalLoadBalancerElement
> To fix that we proposed to create a new class
> “com.cloud.network.element.HAProxyLBRule”, instantiate it with @Componente
> and inject into the aforementioned classes.
> That class will contain the following methods extracted from
> com.cloud.network.element.VirtualRouterElement class:
> com.cloud.network.element.HAProxyLBRule.containsOnlyNumbers(String, String)
> com.cloud.network.element.HAProxyLBRule.validateHAProxyLBRule(LoadBalancingRule)
> However we don't know if all analyzed classes are singletons, if some of
> analyzed classes aren't singletons. Therefore that change could have an
> impact on memory consumption. It will require a deeper analysis to check
> which classes are singleton or not and leave the “logger” as static in such
> classes.
> We have a solution at our own branch
> https://github.com/rafaelweingartner/cloudstack/tree/master-lrg-cs-hackday-003
> However, there is still the need to double-check the singleton problem.
> There is a PR we created in our first attempt to solve that problem:
> https://github.com/apache/cloudstack/pull/714
> The dev that starts working on this ticket can reach us at any moment, hence
> that we have mapped all of ComponentLifecycleBase subclasses that use the
> logger variable.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
