> On Aug. 9, 2016, 12:14 a.m., Peter Vary wrote: > > Hi Zoltan, > > > > Thanks for the patch, I can see, that you were working on it even on the > > weekend. > > > > Please help me to understand the components a little more, so I could help > > with the review. > > As I can see there are 3 levels of the classes for every given test: > > - Configuration > > - Adapter > > - Driver > > > > I have tried to identify the functionality of the given elements, and come > > up with the following: > > - Configuration - The queries to run, the configuration of the clusters, > > and the initial data > > - Adapter - The actual methods for implementing the test, like class, > > method level setup, and test execution > > - Driver - These contain very little code, and they look very simmilar, so > > a lot of code duplication is there - should not be a good idea to merge it > > with the Adapter class? Also it is a little strange, that the Configuration > > has to have a reference to the Adapter. If you decide to merge the Adapter > > and the Driver, then the reference is not needed anymore. > > > > Thanks, > > Peter > > Zoltan Haindrich wrote: > Hello, > > yeah...i wanted to take advantage of the "empty queue" on the ptest > executor ;) > by the way i think that all hive precommit jobs which end-up on ubuntu-3 > will fail with some wierd jdk issue... > > I think you are getting it right...those classes which have "Driver" in > there names are the successors of the old vm files: i don't want to touch > them until we have all of them on board. > There is some redundancy even between the Driver classes...CliDriver and > some others are very similar - it will be easy to drop some of them. > Merge with the adapter would possibly remove the common parent - and that > would possibly break the factory adapterfactory. > The positive side of the current design is that all configurations are in > one place...even the core executor selection is in CliConfigs - so you have > to look at just one place if you have to modify it. > > About more refactoring work: reviewboard can pick-up changes in renamed > files (which is great) - but if I add more refactors to this patch: it will > look like a "20 files remove", "30 files added" - which is not really review > friendly ;) it have already lost track of the changes of PerfCliDriver and > QTestGenTask. > > I would like to continue this with a cleanup refactor; after AccumuloCli > and BeelineCli is on-board. > > regards, > Zoltan
Hi Zoltan, If I were in BP, I would offer you a beer, to discuss this above it :). Unfortunately this is not an option now, so we have to do it on the hard way. What final design do you have in your mind, I think we should discuss these changes in the light of those, and should not focus on partial solutions. For example - correct me if I wrong - the Adapter model is most useful, if there is an existing interface, we have to adhere. So the final design does not require an adapter since the interfaces are used by only the tests, and we could change them if needed. I think we should plan for the following changes, and keep everything else as simple as possible: 1. Adding new queries - this happens very often (maybe too often in my opinion, but let’s not discuss it here :) ) 2. Changing how to handle the specific test case results (ordering/filtering/regexp) - QTestUtil, HBaseQTestUtil, QFileClient for BeeLine 3. Adding new test, to test new integrated components - like it was in case of BeeLine/Spark/Tez Only in the 3rd case should we touch the Driver, and the Adapter, but then we should change both of them. For me it means that they are tightly coupled, and might be a good idea to merge them. What do you think? Thanks, Peter - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/50906/#review145152 ----------------------------------------------------------- On Aug. 8, 2016, 8:47 p.m., Zoltan Haindrich wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/50906/ > ----------------------------------------------------------- > > (Updated Aug. 8, 2016, 8:47 p.m.) > > > Review request for hive. > > > Bugs: HIVE-14444 > https://issues.apache.org/jira/browse/HIVE-14444 > > > Repository: hive-git > > > Description > ------- > > this patch focuses on removing the qtestgen task without introducing > regressions in existing testing services - its not the nicest patch...but > opens the possiblities to continue refactoring of these classes inside a pure > java environment > > this patch contains a bunch of helper classes to provide Tests for the junit4 > executor. > > I've mimiced the old generated testcases behaviour: > > * every test has a config entry in CliConfigs > * every config has a Core executor - these can be looked as the older vm files > * every test has an instance in the appropriate module - this class is > small...copy pasted parameterized junit4 test > > > Diffs > ----- > > ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java > f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b > ant/src/org/apache/hadoop/hive/ant/antlib.xml > 8f663482348448be9aadcf535c42a8c11d8955b1 > hbase-handler/src/test/templates/TestHBaseCliDriver.vm > f513f0374b1e6798677e98b4d071d5969d204bfa > hbase-handler/src/test/templates/TestHBaseNegativeCliDriver.vm > 043bd87a4f617de7fff89e38e654770cd9b84d8f > itests/qtest-accumulo/pom.xml 339c59919295b66c9d3c64a53ea78bd944e3a903 > itests/qtest-spark/pom.xml 3bc9e24893a42bfcaab66d4cb8930dd5586c1db5 > > itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestMiniSparkOnYarnCliDriver.java > PRE-CREATION > > itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestSparkCliDriver.java > PRE-CREATION > > itests/qtest-spark/src/test/java/org/apache/hadoop/hive/cli/TestSparkNegativeCliDriver.java > PRE-CREATION > itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678 > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/ContribNegativeCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/DisabledTestBeeLineDriver.java > PRE-CREATION > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/DummyCliDriver.java > PRE-CREATION > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCompareCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestContribCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestContribNegativeCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestEncryptedHDFSCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseMinimrCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestHBaseNegativeCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMiniLlapCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMiniTezCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestMinimrCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestNegativeCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestNegativeMinimrCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestPerfCliDriver.java > PRE-CREATION > > itests/qtest/src/test/java/org/apache/hadoop/hive/ql/parse/TestParseNegativeDriver.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/AbstractCliConfig.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliAdapter.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfigs.java > PRE-CREATION > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreDummy.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CorePerfCliDriver.java > PRE-CREATION > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java > b3cf6da56eddc5000515e5ebd34989e64b1c0010 > pom.xml 0217edea03b71522629ec30514b67a4a5313edb6 > ql/src/test/templates/TestBeeLineDriver.vm > 563d7fd03600d391b5694cc8477562d9f2e5c9d9 > ql/src/test/templates/TestCliDriver.vm > 0ccedce7361fa87a72f369db90cd9c57dc8b26e2 > ql/src/test/templates/TestCompareCliDriver.vm > 8d4e96437c19177561f1d83ac4f22c16e98bfa93 > ql/src/test/templates/TestNegativeCliDriver.vm > 592d64f80b74ff76e897e2ae07f8635e46675932 > ql/src/test/templates/TestParseNegative.vm > 9500eced31d64e5029d9b2be429aaa6828252f11 > ql/src/test/templates/TestPerfCliDriver.vm > d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a > > Diff: https://reviews.apache.org/r/50906/diff/ > > > Testing > ------- > > some manual tests for qfile, initScript ... they should work as before > > ptests's use of minimr.test.files and such have caught me by surprise...but > those are also supported (i think) > > > Thanks, > > Zoltan Haindrich > >
