[
https://issues.apache.org/jira/browse/CLOUDSTACK-9252?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15127292#comment-15127292
]
ASF GitHub Bot commented on CLOUDSTACK-9252:
--------------------------------------------
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
> Support configurable NFS version for Secondary Storage mounts
> -------------------------------------------------------------
>
> Key: CLOUDSTACK-9252
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9252
> Project: CloudStack
> Issue Type: Bug
> Security Level: Public(Anyone can view this level - this is the
> default.)
> Components: VMware
> Reporter: Nicolas Vazquez
>
> After starting secondary storage VM, secondary storage tries to be mounted
> but fails with error: {{Protocol family not supported}}
> It was found out that adding {{-o vers=X}} to mount command it would work,
> where {{X}} is the desired NFS version to use.
> If it is desired to mount a store with a specific NFS version, it has passed
> in {{image_store_details}} table for a store with id {{Y}} as a property:
> ||store_id||||name||value||
> |Y|nfs.version|X|
> Where X stands for NFS version
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
