> 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 > > Peter Vary wrote: > 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
I'm afraid I don't fully understand your concerns ..but I try to answer what I could: I think these classes are nowhere final now...i'm not even sure how the end result will look like... * in the near future i think some junit rule classes may help cleaning up both the drivers and the deeper qtest related features as well * future of this adapter named class is unknown...it may or may not be removed later. Its name is adapter because there was a non declared interface which was used in all clidrivers; and I wanted to package those things into junit4 rules + add checks to see if any of them named the method a bit differently; i can rename it to AbtractCliDriver if it causes confusion. * 1,2 is I think outside of the scope of this ticket. * (3) i think that having some common way to configure these test is an advantage....so I think you shouldn't change anything beyond adding a new driver; and a config. cheers, Zoltan - Zoltan ----------------------------------------------------------- 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 > >
