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 [email protected] or file a JIRA ticket
with INFRA.
---
