hive-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Zoltan Haindrich <k...@rxd.hu>
Subject Re: Review Request 50906: HIVE-14444 Upgrade qtest execution framework to junit4 - migrate most of them
Date Wed, 10 Aug 2016 11:47:38 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message