[
https://issues.apache.org/jira/browse/CLOUDSTACK-9298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15176576#comment-15176576
]
ASF GitHub Bot commented on CLOUDSTACK-9298:
--------------------------------------------
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1425#issuecomment-191455410
@nvazquez I have reviewed your code.
Class “VolumeJoinDaoImpl” at line 46, why did you change the get Logger
method to use “VolumeJoinDaoImplTest”?
The method “updateTemplateTagInformation” is duplicated (I would say
triplicated) at “VolumeJoinDaoImpl”, “UserVmJoinDaoImpl” and
“TemplateJoinDaoImpl”. I know, you may argue that the method
“updateTemplateTagInformation” has a conditional in “TemplateJoinDaoImpl” and
“VolumeJoinDaoImpl”; still, that conditional is executed with some additional
checks in “UserVmJoinDaoImpl”. The point here is that you should let that
conditional out of the method scope, and then extract the method to a place
that can be reused in all of those classes. All of them (VolumeJoinDaoImpl,
UserVmJoinDaoImpl, and TemplateJoinDaoImpl) extend “GenericDaoBase” class. You
can easily create a new class that extend “GenericDaoBase”, with that
“updateTemplateTagInformation” and make all of those classes extend this new
one. Please, keep that in mind in your next PRs.
That same is happening for VolumeJoinVO, UserVmJoinVO, and TemplateJoinVO
classes. All of them have attributes “tagAccountName”, “tagDomainUuid” and
“tagDomainName” and their respective getters and setters. They all extend
“BaseViewVO”, you can do that same trick I mentioned you above here.
Now about the tests. First, when writing tests, you should ask yourself,
“What am I going to test?”, “If this method changes the behavior would I like
to catch it?”, “What is the behavior I want to monitor the behavior of?”. Those
are the basic questions.
I believe your tests, should test the method “updateTemplateTagInformation”
that you wrote. So, what do you want to detect if that method stops setting a
property? Do you want to detect if it sets a property in a wrong place? If you
want that, your tests now are not adequate, if you do not want that, you should
delete the tests classes.
I personally believe that the method deserves tests, to check if it is
working as expected. Meaning, that it sets the properties you want in the right
place.
Additionally, when writing tests try to code as little as possible.
Variables “accountService” and “configDao” are not needed to test the
“updateTemplateTagInformation”; keep in mind that is a unit test. To test
“updateTemplateTagInformation” you only need a simple instance of
“TemplateJoinDaoImpl” and a mock of “ApiDBUtils.newResourceTagResponse(vtag,
false)”
> Improve performance of resource retrieval that have tags associated and
> target volumes, VMs and templates
> ---------------------------------------------------------------------------------------------------------
>
> Key: CLOUDSTACK-9298
> URL: https://issues.apache.org/jira/browse/CLOUDSTACK-9298
> Project: CloudStack
> Issue Type: Improvement
> Security Level: Public(Anyone can view this level - this is the
> default.)
> Components: API
> Affects Versions: 4.9.0
> Reporter: Nicolas Vazquez
> Fix For: 4.9.0
>
>
> h2. Description of the problem
> When retrieving a large number of resources which have tags associated with,
> retrieval methods took too long. Our goal is to improve performance of this
> methods
> h3. ListTemplatesCmd API method
> It is proposed to include tags information into template_view to avoid
> querying the database for each tag, managing that information in memory.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
