cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <>
Subject [jira] [Commented] (CLOUDSTACK-9298) Improve performance of resource retrieval that have tags associated and target volumes, VMs and templates
Date Wed, 02 Mar 2016 22:09:18 GMT


ASF GitHub Bot commented on CLOUDSTACK-9298:

Github user rafaelweingartner commented on the pull request:
    @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,

> Improve performance of resource retrieval that have tags associated and target volumes,
VMs and templates
> ---------------------------------------------------------------------------------------------------------
>                 Key: CLOUDSTACK-9298
>                 URL:
>             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

View raw message