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