You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by "Rakesh Radhakrishnan (Jira)" <ji...@apache.org> on 2021/04/26 16:53:01 UTC

[jira] [Assigned] (HDDS-5097) [FSO] Cleanup integration tests and reduce the execution time

     [ https://issues.apache.org/jira/browse/HDDS-5097?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]

Rakesh Radhakrishnan reassigned HDDS-5097:
------------------------------------------

    Assignee: Rakesh Radhakrishnan

> [FSO] Cleanup integration tests and reduce the execution time 
> --------------------------------------------------------------
>
>                 Key: HDDS-5097
>                 URL: https://issues.apache.org/jira/browse/HDDS-5097
>             Project: Apache Ozone
>          Issue Type: Sub-task
>            Reporter: Marton Elek
>            Assignee: Rakesh Radhakrishnan
>            Priority: Blocker
>              Labels: pull-request-available
>
> Apache organization has a shared, organization-level resource limit for Github actions. All the projects should use the available minutes fairly to avoid blocking the build of other Apache projects.
> This is discussed recently here: https://lists.apache.org/thread.html/r48d079eeff292254db22705c8ef8618f87ff7adc68d56c4e5d0b4105%40%3Cbuilds.apache.org%3E, but same mailing list has older threads, too.
> HDDS-2939 branch build time has been increased significantly. The problem with acceptance tests are tracked with HDDS-5093. 
> But integration tests are also increased from ~56 minutes to 94 minutes (~158%)
> We need to avoid such increase unless we have very good reason to have it.
> If we check the execution time of the tests on the branch and the master we can see the problem in more details:
> {code}
>  tmp  %  grep -e 'Tests run: .* in org' "20" | awk -F '[,:\-]' '{print $6, $8, $10, $12, $14, $15}' | sed 's/s   in //g' | sort -n -k5 -r | head
> awk: warning: escape sequence `\-' treated as plain `-'
>  104  0  0  0  478.951 org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
>  88  0  0  0  458.703 org.apache.hadoop.fs.ozone.TestOzoneFileSystem
>  18  0  0  2  436.521 org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
>  6  0  0  0  152.808 org.apache.hadoop.fs.ozone.TestOzoneFSWithObjectStoreCreate
>  4  0  0  0  108.191 org.apache.hadoop.hdds.scm.pipeline.TestPipelineClose
>  1  0  0  0  88.905 org.apache.hadoop.hdds.scm.pipeline.TestNodeFailure
>  2  0  0  0  74.192 org.apache.hadoop.fs.ozone.TestOzoneFsHAURLs
>  19  0  0  0  58.2 org.apache.hadoop.fs.ozone.contract.rooted.ITestRootedOzoneContractGetFileStatus
>  8  0  0  0  57.063 org.apache.hadoop.fs.ozone.contract.rooted.ITestRootedOzoneContractDistCp
>  8  0  0  0  54.612 org.apache.hadoop.fs.ozone.contract.ITestOzoneContractDistCp
>  tmp  %  grep -e 'Tests run: .* in org' "18" | awk -F '[,:\-]' '{print $6, $8, $10, $12, $14, $15}' | sed 's/s   in //g' | sort -n -k5 -r | head
> awk: warning: escape sequence `\-' treated as plain `-'
>  128  0  0  0  509.388 org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystem
>  132  0  0  0  508.585 org.apache.hadoop.fs.ozone.TestOzoneFileSystem
>  140  0  0  8  479.578 org.apache.hadoop.fs.ozone.TestOzoneFileSystemV1
>  18  0  0  2  475.686 org.apache.hadoop.fs.ozone.TestOzoneFileInterfaces
>  128  0  0  0  470.619 org.apache.hadoop.fs.ozone.TestRootedOzoneFileSystemV1
>  18  0  0  6  329.812 org.apache.hadoop.fs.ozone.TestOzoneFileInterfacesV1
>  5  0  0  0  226.093 org.apache.hadoop.fs.ozone.TestDirectoryDeletingServiceWithFSOBucket
>  6  0  0  0  171.861 org.apache.hadoop.fs.ozone.TestOzoneFSWithObjectStoreCreate
>  4  0  0  0  139.951 org.apache.hadoop.hdds.scm.pipeline.TestPipelineClose
>  3  0  0  0  82.128 org.apache.hadoop.fs.ozone.TestOzoneFileSystemPrefixParser
> {code}
> It turned out that a lot of tests just copied with V1 prefix  with small modification  (or original test extended with V1 which means  the methods of the old and new tests are executed) instead of improving the original test to cover both of the cases (simple/prefix-ed).
> Also: the same functionality seems to be tested on multiple levels (FileSystemInterface, OMRequest, acceptance test...) 
> This copy can make the maintenance of the tests slightly harder, and the V1 prefix is quite meaningless and confusing.
> From the legendary "Clean code" book:
> >Programmers create problems for themselves when they write code solely to satisfy a compiler or interpreter. For example, because you can’t use the same name to refer to two different things in the same scope, you might be tempted to change one name in an arbitrary way. Sometimes this is done by misspelling one, leading to the surprising situation where correcting spelling errors leads to an inability to compile.2
> > [...] It is not sufficient to add number series or noise words, even though the compiler is satisfied. If names must be different, then they should also mean something different.
> > Number-series naming (a1, a2, .. aN) is the opposite of intentional naming. Such names are not disinformative—they are noninformative; they provide no clue to the author’s intention....
> I totally agree with this section, I think we should avoid using V1 prefixes everywhere.
> Suggestions:
>  1. We should minimize the additional build time when possible.
>  2. Similar to HDDS-5093 instead of creating a new dimension to the parametrized tests (which double the execution time) we should carefully select the meaningful sets of parameters. (When we have 3 parameters for a unit test with 2 values for each it's already 8 possible executions. Execution all of them with and without prefix may not be required, it seems to be better to select representative parameter sets and use only them).
>  3. The V1 prefixes should be avoided. When possible we need to modify the original unit tests with adding additional parameters. It makes it easier to maintain the unit tests, and it requires less code and (together with the previous point) less time  
>  4. We don't need to repeat ALL the tests IMHO. For example if we modified only the rename part of the OzoneFileSystem, I assume that it's enough to test that part with/without prefix (especially as the prefix handling is already tested with testing om requests...)
>  



--
This message was sent by Atlassian Jira
(v8.3.4#803005)

---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org