Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============2160957157153532721=="
MIME-Version: 1.0
Subject: Re: Review Request 51397: HIVE-14536 Unit test code cleanup
From: Barna Zsombor Klara <zsombor.klara@cloudera.com>
To: Zoltan Haindrich <kirk@rxd.hu>,
 Miklos Csanady <miklos.csanady@cloudera.com>,
 Siddharth Seth <sseth@apache.org>,
 Barna Zsombor Klara <zsombor.klara@cloudera.com>,
 Ashutosh Chauhan <hashutosh@apache.org>, Marta Kuczora <kuczoram@cloudera.com>,
 j.prasanth.j@gmail.com, Sergio Pena <sergio.pena@cloudera.com>,
 Sergey Shelukhin <sergey@hortonworks.com>,
 Gabor Szadovszky <gabor.szadovszky@cloudera.com>
Cc: Peter Vary <pvary@cloudera.com>, hive <dev@hive.apache.org>
Date: Thu, 25 Aug 2016 16:55:43 -0000
Message-ID: <20160825165543.2089.9180@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Barna Zsombor Klara <noreply@reviews.apache.org>
References: <20160825145019.2090.63433@reviews.apache.org>
In-Reply-To: <20160825145019.2090.63433@reviews.apache.org>
Reply-To: Barna Zsombor Klara <zsombor.klara@cloudera.com>
archived-at: Thu, 25 Aug 2016 16:55:48 -0000

--===============2160957157153532721==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51397/#review146792
-----------------------------------------------------------


itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 186)
<https://reviews.apache.org/r/51397/#comment213469>

    nit: Could we add some javadoc explaining why it is deprecated and what to use instead?
    And if we are adding javadoc then we could document what it is good for to begin with. :)


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 216)
<https://reviews.apache.org/r/51397/#comment213474>

    nit: Could the return type be Set instead of the implementation?


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2272)
<https://reviews.apache.org/r/51397/#comment213479>

    Nit: Can we decide on whether to use System.out or System.err for logging? We are using the two seemingly randomly to log out messages and errors between methods.


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2284)
<https://reviews.apache.org/r/51397/#comment213484>

    Can we add javadoc to runQuery, runFailingQuery, runTest, runVersionedTest, runParseTest, so all the public methods which aren't trivial getters?


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2315)
<https://reviews.apache.org/r/51397/#comment213480>

    Nit: Catching exceptions is probably enough.


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2366)
<https://reviews.apache.org/r/51397/#comment213485>

    nit: Catching exceptions is probably enough.


itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2399)
<https://reviews.apache.org/r/51397/#comment213483>

    nit: Catching exceptions is probably enough.


Minor comments, LGTM otherwise.
Thanks for the patch.

- Barna Zsombor Klara


On Aug. 25, 2016, 2:50 p.m., Peter Vary wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51397/
> -----------------------------------------------------------
> 
> (Updated Aug. 25, 2016, 2:50 p.m.)
> 
> 
> Review request for hive, Ashutosh Chauhan, Gabor Szadovszky, Zoltan Haindrich, Marta Kuczora, Miklos Csanady, Prasanth_J, Sergey Shelukhin, Sergio Pena, Siddharth Seth, and Barna Zsombor Klara.
> 
> 
> Bugs: HIVE-14536
>     https://issues.apache.org/jira/browse/HIVE-14536
> 
> 
> Repository: hive-git
> 
> 
> Description
> -------
> 
> Cleaning up the CliDrivers with the following requirements:
> - If there is a problem with a specific testcase, it should be trivial to find the corresponding methods that had been running
> - Later it should be possible to run the testcases parallel
> - No test result changes in this patch, so validation should be easier
> - The QTestUtil classes not refactored - only added functionality which belongs there - later could be cleaned up as well
> 
> The selected "architecture"
> - CliConfig class to store the configurations
> - Testcases without inheritance - every beforeclass, before, after, afterclass should be in this same file
> - Repeating codes refactored to the QTestUtil classes
> 
> Beeline driver - created, compiling, but removed the test annotations since none of the test output files are valid even with the current version - later should be cleaned up
> Accumulo driver - created, compiling, 3 of the tests are ok, another 3 tests was failing before. Currently this version does the same - later should be cleaned up
> 
> Open for any suggestions, feel free to criticize!
> 
> 
> Diffs
> -----
> 
>   itests/qtest-accumulo/src/test/java/org/apache/hadoop/hive/cli/TestAccumuloCliDriver.java bf50f16 
>   itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestMiniSparkOnYarnCliDriver.java e84bfce 
>   itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestSparkCliDriver.java 2c8cbee 
>   itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestSparkNegativeCliDriver.java 2db83f4 
>   itests/qtest/pom.xml ed44bb8 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/ContribNegativeCliDriver.java 253cda3 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/DisabledTestBeeLineDriver.java cb276e6 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/DummyCliDriver.java 965d1dc 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestBeeLineDriver.java PRE-CREATION 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCliDriver.java c4c4f41 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCompareCliDriver.java 944cd32 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestContribCliDriver.java 54596f9 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestContribNegativeCliDriver.java 1b39ee7 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestEncryptedHDFSCliDriver.java 8c6807e 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseCliDriver.java 7b6f76a 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseMinimrCliDriver.java 934af16 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseNegativeCliDriver.java 88d626c 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMiniLlapCliDriver.java ad525fe 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMiniTezCliDriver.java c23b0b3 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMinimrCliDriver.java 96a9e8f 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestNegativeCliDriver.java 1040228 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestNegativeMinimrCliDriver.java f7e2caa 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestPerfCliDriver.java 4df4eeb 
>   itests/qtest/src/test/java/org/apache/hadoop/hive/ql/parse/TestParseNegativeDriver.java 4c1224f 
>   itests/util/src/main/java/org/apache/hadoop/hive/accumulo/AccumuloQTestUtil.java 88bc0bc 
>   itests/util/src/main/java/org/apache/hadoop/hive/accumulo/AccumuloTestSetup.java 73d5f15 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/AbstractCliConfig.java efbd465 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliAdapter.java b89d6e7 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java PRE-CREATION 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java 319a205 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreAccumuloCliDriver.java a5d2711 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java e5144e3 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreCliDriver.java 5435f9f 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreCompareCliDriver.java 71a02bc 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreDummy.java b7afb48 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreHBaseCliDriver.java 956a42d 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreHBaseNegativeCliDriver.java 6225180 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreNegativeCliDriver.java 65b2ce7 
>   itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CorePerfCliDriver.java 8620cde 
>   itests/util/src/main/java/org/apache/hadoop/hive/hbase/HBaseQTestUtil.java 01faaba 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java 358ba51 
>   itests/util/src/main/java/org/apache/hadoop/hive/ql/parse/CoreParseNegative.java 8dba0bb 
> 
> Diff: https://reviews.apache.org/r/51397/diff/
> 
> 
> Testing
> -------
> 
> Run the test cases on a single machine.
> At least 20 for ever Driver (at least 10 miniutes each).
> The results were the same as for the runs without the patch.
> Checked the number of the selected queryfiles, and it is matching with the current number
> Run the testcases from intellij, there were some problems (missing TEST_HADOOP_CLASSPATH), but most of the testcases/queries are ok.
> Waiting for the QA, to validate the test results and I will update the patch if needed
> 
> 
> Thanks,
> 
> Peter Vary
> 
>


--===============2160957157153532721==--