cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From rafaelweingartner <...@git.apache.org>
Subject [GitHub] cloudstack pull request: CLOUDSTACK-9252: Support configurable NFS...
Date Tue, 02 Feb 2016 00:04:45 GMT
Github user rafaelweingartner commented on the pull request:

    https://github.com/apache/cloudstack/pull/1361#issuecomment-178267025
  
    Hi @nvazquez, 
    I see that you have created a spring bean as I said. However, I noticed some things that
may not be needed or that seemed out of place to me. Before I start pointing them out, do
you know how spring works? It is not a problem not to know in depth this kind of framework,
but if you do not I could prepare some explanation in detail, so it facilitates in the future
for you.
    
    At the “spring-engine-storage-image-core-context.xml” line 31, I noticed that you
added the bean “imageStoreDetailsUtilImpl” as a dependency of the “templateServiceImpl”
bean; that is not necessary, if the bean “templateServiceImpl” has a property of type
“imageStoreDetailsUtilImpl” annotated with "@Inject/@Autowired", Spring will automatically
sort and create the hierarchy of beans dependencies to instantiate and inject. That “depends-on”
configuration can be used for some things that are slightly different; if you are curious
about that we can chat in off, just call me on slack or shoot me an email.
    
    Still on “spring-engine-storage-image-core-context.xml” at line 40, you declared the
bean “imageStoreDetailsUtilImpl”, that is only necessary if you were not using the “@Component”
annotation (there are others that you could use too, such as @Service, @Bean and others, each
one to mark a different use type of bean); having said that, there is no need to declare the
bean in the XML.
    
     If you tried to build and run the ACS with your changes, but ended up with the application
not going up because some problem with Spring dependency resolution; that might have happened
because of ACS application contexts hierarchy, If that happened I can help you find the best
place to declare the bean; otherwise, you can just use the annotation that is fine, no need
to re-declare the bean in an XML file. I do not think that problem will happen because the
bean will be created at one of the top-level application contexts of ACS.
    
    Now about the bean itself; I noticed that you created an interface to declare the bean's
methods. Normally, when we are creating DAO or service classes that is the right way to do
things, create an interface and then the implementation, allowing to use object orientation
(O.O.) to change the implementation in the future with configurations in an XML file; (an
opinion) this is not the same as creating code for the future, but it is preparing the architecture
of a system for the future, I dislike the first one and like the second one. However, with
the use of annotations, it is not that easy to change implementation as it is when using XML
spring beans declaration; it is not possible to inject a different object that implemented
the same interface, since annotations make everything pretty straightforward, so I think it
is better to lose the interface and work just with a single class that is the component itself.
    
    Additionally, I noticed that in your interface (that I suggest you not to use in this
specific case) you extended the interface “Manager” that brings a lot of things that you
do not use, I am guessing you did that because you have seen some other classes, and they
all do that. Well, guess what, in your case that is not needed. Actually, in most cases that
the "Manager" interface is being used that interface is not needed; I find the “Manager”
interface hierarchy a real nightmare, but that is a topic for another chat. 
    
    In all of the places you injected the” imageStoreDetailsUtil” object, I suggest you
removing the “_” from the attribute name and making them private.
    
    Now the problem with poor application architecture planning appears (it is not your fault,
you are actually doing a great job). In some “service/manager/others” that should work
as singletons, but are not, your “@Inject” will not work.
    These classes “VmwareStorageManagerImpl”, “VmwareStorageProcessor”, “VmwareStorageSubsystemCommandHandler”
and “DownloadManagerImpl” are manually instantiated (I might have missed some other),
which means that they do not pass through the Spring framework lifecycle, which means that
@Inject on them will not work. To transform them into Spring managed objects, it would require
much more effort than you should be doing (you are already doing applying a great effort on
this). What you can do is to get that bean during the object initialization at their constructor;
you can use the “com.cloud.utils.component.ComponentContext.getApplicationContext()” to
retrieve the Spring application context, and then the getBean method, to retrieve the desired
bean; then you are good to set that bean in an attribute of the object and use it as you are
now. Just do not forget to remove the @Inject from the aforementioned classes.
     
    I recommend you after applying those changes, you should try to build and start up the
application (ACS with your PR) to check if everything is getting injected and is working as
expected. If you run into any problems, just call me.
    
    @nvazquez, that is the way to do it, unit tests using mock DAOs, you are on the right
track ;)
    
    BTW: I liked very much your code, small methods, with test cases (still to come) and java
doc


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

Mime
View raw message