Mailing-List: contact dev-help@cloudstack.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@cloudstack.apache.org
From: rafaelweingartner <git@git.apache.org>
To: dev@cloudstack.apache.org
Reply-To: dev@cloudstack.apache.org
References: <git-pr-1361-cloudstack@git.apache.org>
In-Reply-To: <git-pr-1361-cloudstack@git.apache.org>
Subject: [GitHub] cloudstack pull request: CLOUDSTACK-9252: Support
 configurable NFS...
Content-Type: text/plain
Message-Id: <20160202000445.97278DFCE4@git1-us-west.apache.org>
Date: Tue,  2 Feb 2016 00:04:45 +0000 (UTC)

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