> On Aug. 26, 2016, 2:01 a.m., Siddharth Seth wrote: > > Not a lot here related to the patch itself in this comment. > > Looks like everyone has gotten interested in fixing Hive tests at about the > > same time - and there's a good reason for that :) > > I think it would be better to get changes in via smaller patches. This one > > is at 180KB already (without touching q files). This is both to make it > > easier to review, and to have a smaller window for conflicts. There's > > several jiras open to fix either testing infrastructure, or individual > > tests, or get things working in the IDE. I know Zoltan was driving towards > > getting tests working via the IDE - I don't know how this affects that > > effort. > > Peter Vary wrote: > Hi Siddharth, > > With Zoltan, we have the same goals, and with his previous patch I was > able to run query tests in IDE - in fact I made most of my tests from the > IDE, and falled back to mvn only to validate that it is still working. So he > made a good progress on that. > On the review of one of his last patch, we agreed on jira comments > (HIVE-14444), that he will commit his work, and later I will try to remove > some of the technical debt from the testframework. > This change is big alone, and I made the mistake to add some minor stuff > there, where it made my life miserable during the testing: > - Making the BeeLine test able to compile > - Removing created UDF-s in the refresh method > - Merged/added HIVE-14625 changes to the patch since it updated files I > removed/changed > > I could remove those changes, if you think it would be easier to review, > but I think the complexity more in the bigger part, which could not be > splitted - touching every CliDriver classes, Adapters, QTestUtils. > > What I think is a good indicator is the results of the QA run. The patch > run the same amount of tests as before, and resulted in the same errors as > before. So I am quite positive that we have made progress to the good > direction, but of course the decision is up to the reviewers. > > Thanks, > Peter > > Siddharth Seth wrote: > It's great that all the tests continue to pass after the change, and we > have some additional functionality around UDFs, beeline etc. > As you pointed out, there's multiple things being changed in the patch. > 1. A refactor of the recent refactor for the test framework > 2. Rmoving created UDFs > 3. Beeline tests compiling. > 4. Some refactoring in QTestUtil? > > These really should be split into separate jiras. The remove UDF change > can likely be reviewed in isolation and committed. Per the descsription, > refactor of repeating code in QTestUtil - can easily be a separate jira. The > large refactor already has a large number of review comments - and causes > each independent change to slow down. Meanwhile, others make changes to > QTestUtils, and the patch needs to be refacatored. > > From a quick glance at the patch. > - I'm not in favor of moving more code into QTestUtil - especially since > it increases the potential for code abuse by maniplating private vars. > QTestUtil is complicated enough as is. > - Also curious about how the current custom framework vs the setup after > the refactor affects adding timeouts to individual tests. This would seem > easy. However, Hive has a lot of thread local context which gets in the way > since junit launches a separate thread to enforce the timer afaik. Would the > custom framework make controlling these threads any simpler? > - Are there any advantages of the new approach in terms of exception > reporting? (I'll run it locally sometime to find out)
Removed the independent changes - patch file size down to ~180k, smaller, than Zoltan's previous patch :) - I agree, that the QTestUtil with its many private variables is a problem. After this patch, I plan to clean that up too, if you agree. There we can decide if it is better to split it to several different classes, or we can keep it clean with one class. - I think both before, and after the refactor, it is only possible to set the timeout on the test method, so every query run in the same Driver will have the same timeout. Please Zoltan correct me if I am wrong here regarding the current framework. We have to write our own Runner to set it on query file basis - There is fewer noise on the exceptions after the refactor - Every information was there before, but no extra exception generated on the top of the root cause - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51397/#review146909 ----------------------------------------------------------- On Aug. 26, 2016, 2:38 a.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51397/ > ----------------------------------------------------------- > > (Updated Aug. 26, 2016, 2:38 a.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 > db58f1d > > 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 4d4a929 > > itests/util/src/main/java/org/apache/hadoop/hive/ql/parse/CoreParseNegative.java > 8dba0bb > pom.xml 9ed1c19 > > 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 > >
