hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Barna Zsombor Klara <zsombor.kl...@cloudera.com>
Subject Re: Review Request 51397: HIVE-14536 Unit test code cleanup
Date Thu, 25 Aug 2016 16:55:43 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message