cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <>
Subject [jira] [Commented] (CLOUDSTACK-8750) Change the variable s_logger to non-static and fixed its name in “” and its subclasses
Date Tue, 15 Sep 2015 18:39:46 GMT


ASF GitHub Bot commented on CLOUDSTACK-8750:

Github user miguelaferreira commented on the pull request:
    I do agree that prepending "s_" to static variables is a waste of characters. Modern IDEs
will signal static variables for you. It's a bit like prepending (or appending) type information
in the variable name. There was a time it was useful, but that time is long gone.
    I also appreciate abstraction in code, that is, you've now introduced a protected variable
in a base class that will be available to all the classes that extend it. However I do not
see a real benefit in adding this particular indirection level. I mean, loggers are very straight
forward to add and understand, so standardising on they declaration and use, seem to me far
better than having it be a static variable in some classes, and a non static-protected-inherited
variable in others.
    There is plenty I would like see done in terms of cleaning the code of ACS. For instance
I've measured duplicate code a year ago and found 25% of the code to be redundant. 
    The DB layer is another example. ACS is at the moment using a deprecated library for it's
DB layer, while there are other libraries (e.g. spring-data) that would actually generate
most of the hand-made code we have now.
    I'll be very happy to share my thoughts with you about potential refactorings that would
benefit the ACS code.

> Change the variable s_logger to non-static and fixed its name in “”
and its subclasses
> --------------------------------------------------------------------------------------------------------------------------------------
>                 Key: CLOUDSTACK-8750
>                 URL:
>             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 “”
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 “”
to protected and remove its static and final declaration. Therefore we did the following in
>    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:
> To fix that we proposed to create a new class 
> “”, instantiate it with @Componente and inject
into the aforementioned classes.
> That class will contain the following methods extracted from
>, String)
> 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 
> 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:
> 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

View raw message