cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-8750) Change the variable s_logger to non-static and fixed its name in “com.cloud.utils.component.ComponentLifecycleBase” and its subclasses
Date Wed, 16 Sep 2015 12:38:46 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-8750?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14768863#comment-14768863
] 

ASF GitHub Bot commented on CLOUDSTACK-8750:
--------------------------------------------

Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/778#issuecomment-140729409
  
    Dear @wilderrodrigues, As I said I get your point and the moment that you guys are on
closing a version. 
    
    A mistake was made, the moment I noticed that I jumped in and checked the problem and
tried to fix it with this PR. No need to cut someone’s head with adamantium claws.
    
    Sure the task was huge, but simple. That was the point on giving it to a less experienced
student. I want them to see that even in huge systems like CloudStack and others; there are
codes that are not pretty and shiny.
    
    The improvement factor depends on the concept you use, removing few lines of code and
using a more intuitive and standard name for a variable in a hierarchic of classes can be
considered an improvement.  The same occur if we remove the @Local annotation of that hierarchic,
the task will affect 600+ classes, but it is a simple one.
    
    Since you said that there is no way this PR will get accepted, I am closing it.
    Thanks for your time.



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

Mime
View raw message