> 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 > > Zoltan Haindrich wrote: > 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
Hi Zoltan, Here is what I would do: - Create CliConfig.java - same as the proposed AbstractCliConfig without the adapter reference - It has the responsibility to store the configuration, and calculate the queries to run (I think the exact methods of setting up, and tearing down the test infrastructure is not part of the configuration - it is part of the testcase) - Create an AbstractXXXCliDriver class from the vm templates only if the template is currently used with multiple test configurations in the pom.xml-s (TestCliDriver.vm, TestNegativeCliDriver.vm, TestHBaseCliDriver.vm) - It has the responsibility to implement the @beforeClass, @afterClass, @before, @after, runtest methods, since they are the same for the given test cases. If the exact implementation details might be refactored to the helper classes like QTestUtil, QFileClient, then these abstract classes might not be needed at all. - Create a TestXXXCliDriver for every test configuration in the pom.xml-s, inherit a TestXXXCliDriver class from the AbstractXXXCliDriver class where there are multiple test cases in the pom.xml-s for a given vm template. This test class should initialize the CliConfig with the appropriate parameters. If there is no parent class, then the TestXXXCliDriver class should take the responsibility to implement the @beforeClass, @afterClass, @before, @after, runtest methods too. I think my proposed solution have the following advantages: - If there is a problem with a TestCase, then every relevant information is in the TestXXXCliDriver class, or in its direct parent class. I think it is more natural to look for it there. - If we have to create a new TestCase, we have to create only one new class with either inheriting from the relevant AbstractXXXCliDriver, or creating a new TestXXXCliDriver class, instead of creating a new Configuration in CliConfigs class, and the TestCase, and maybe a new Adapter as well. - Fewer classes - 1 config class, 3 AbstractXXXCliDriver class, 21 TestXXXCliDriver class (sum: 25) instead of 33+33 inner classes - Simpler object hierarchy: Testcase with a configuration, in some cases with a parent instead of a TestCase with an adapter, and a configuration with an adapter, and an adapter with a configuration That is why I still think this proposed solution should be more maintainable in the long run. 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 > >
