----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51397/#review146818 -----------------------------------------------------------
This patch is huge...it was very time consuming doing its review - it would have been much better to split some of it into smaller changes itests/qtest-accumulo/src/test/java/org/apache/hadoop/hive/cli/TestAccumuloCliDriver.java (lines 43 - 50) <https://reviews.apache.org/r/51397/#comment213560> i think the previous one was more descriptive about what is really happening...by that dsl-alike style ; because the review board was unable to match those lines i copy one here: {code} setQueryDir("ql/src/test/queries/clientpositive"); includesFrom(testConfigProps, "minimr.query.files"); setResultsDir("ql/src/test/results/clientpositive"); setLogDir("itests/qtest/target/qfile-results/clientpositive"); setInitScript("q_test_init.sql"); setCleanupScript("q_test_cleanup.sql"); setHiveConfDir(""); setClusterType(MiniClusterType.mr); {code} i think this is better than having a constructor with 9 arguments.. itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestBeeLineDriver.java (lines 37 - 39) <https://reviews.apache.org/r/51397/#comment213647> this is not seems like a junit test can you add the annotations + @Ignore("will be enabled soon ;)") itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCompareCliDriver.java (line 80) <https://reviews.apache.org/r/51397/#comment213562> i think it would be better to move out "things" from QTestUtil instead of adding more... itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 42) <https://reviews.apache.org/r/51397/#comment213598> the earlier abstractcliconfig class was immutable...and it needed a real descendtant to configure it properly... but it served a purpose: it prevented any furter changes once it had been constructed - and would force anyone wanting to alter the configuration to look for the right place...I've seen value in that approach...but I might be wrong of course... itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 70) <https://reviews.apache.org/r/51397/#comment213610> i'm afraid this is a bit misleading: AFAIK it has nothing to do with the used hadoop...there are only hadoop version dependent excludes and some file naming tricks this thing may twist itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (lines 90 - 97) <https://reviews.apache.org/r/51397/#comment213600> i think no one will really use these new properties...and that last resort default is totally wrong; an exception would be more appropriate - it will not be usable from maven because i think these are not passed in pom.xml itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (lines 101 - 106) <https://reviews.apache.org/r/51397/#comment213601> the result of this block is either the passed metastoreType argument ; or sql, but the property value is never get used.. but either way i think no one will use the property itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 111) <https://reviews.apache.org/r/51397/#comment213523> I think this valueForString method is bad...and I wanted to get rid of it...as it does has a fallback when you do a typo...which will do cause unexpected results. you have removed my FIXME about that issue...i've only kept it before; because one of the values was parsed from a different string by this that valueForString...and I didn't wanted to risk breaking someone elses setup with that patch itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (lines 118 - 122) <https://reviews.apache.org/r/51397/#comment213559> these defaults are not really usefull...i think an exception would be more appropriate in case of initScript of cleanupScript is missing itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 138) <https://reviews.apache.org/r/51397/#comment213603> try-with-resources itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 260) <https://reviews.apache.org/r/51397/#comment213606> i think it would be better to split this into 2 methods: above this line is getQueryFiles belove this is getTestParameters itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 275) <https://reviews.apache.org/r/51397/#comment213569> i think the earlier small notice about that a property override have happend was usefull in the test outputs itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (lines 312 - 313) <https://reviews.apache.org/r/51397/#comment213566> i dont think these various properties will be used by anyone...this one's name is an enum value ;) anyway i think anyone can already override what he needs by using qfile/qfile_regex ... itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (line 348) <https://reviews.apache.org/r/51397/#comment213599> previously we didn't needed a confusing argument like this includeAll... i think it would be better to force the user to supply excludes prior to includes or someething...what could even itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java (lines 382 - 396) <https://reviews.apache.org/r/51397/#comment213590> i think this enum could possibly do more "work" by parsing the property file and serving its contents in usable form itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (lines 216 - 234) <https://reviews.apache.org/r/51397/#comment213617> i failed to find any earlier version of this feature...it looks like a new feature which will drop functions created during testing... anyway...i think this property to control which one to protect should be left out - I think no one will use it. dropping any unknown function is a good idea! itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2259) <https://reviews.apache.org/r/51397/#comment213580> i think the word calculate would involve some artithmetics (just joking ;) I think a getX method may do anything as long it can provide X ;) anyway...this method is superior compared to the config level static setting - and that could be possibly removed.. but AFAIK hadoop-2 is the only one supported now, so this whole thing might get removed later itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (lines 2265 - 2279) <https://reviews.apache.org/r/51397/#comment213581> i think all these sysouts should be exceptions (as before)...because when this happens: the lookup strategy failed -> but that possibly also mean that the whole classpath is crap...or this method is broken...but either way I don't think we may continue without throwing an exception - I think avoiding exceptions like this will just raise debugging time. why have you removed the exceptions? itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 2279) <https://reviews.apache.org/r/51397/#comment213584> i think this catch block in its current form is just some unreachable code I think it would have been better to do this a bit later...because the previous classes (even thru had their own problems) - have aided the refactoring of these things; after this it would be trickier to extract those junit rules from qtestutil and its descendants... There are quite a few changes beyond the refactor: * pom.xml dependency change because of compilation of the beeline test * some new udf protection mechanism * merge of negative check with positive * lots of new system properties not that i'm against these changes, but looking at them separatly in a review friendly context is much easier, faster...and may produce better feedbacks because of the x files removed y files added nature of this patch - today I had to look at files from git to see what have changed... I think these various new properties will never be used...so we should avoid introducing them I think we should concentrate more on getting rid of the various properties and keep only the useful ones; the massive property usage is one of the main reasons why executing tests inside an IDE is hard. I'm still not entirely convinced that removing the template alike nature of these tests is the best way to go...i'm afraid that these separate tests will diverge more and more after a while and there will be fixes which other tests may not get - this have been already present inside the vm files prior to these changes: accumulocli for example does a full qtest reinit after every case executed hbasenegativecli have missed a lot of improvments hbasecli already had ; a single line TestCli change in the earlier one - new equivalent will be to do copy-pasting it to ~10 classes...or just add it to one...and diverge cheers.kirk - Zoltan Haindrich 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 > >
