Mailing-List: contact dev-help@hive.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@hive.apache.org
Content-Type: multipart/alternative;
 boundary="===============5331892836499968643=="
MIME-Version: 1.0
Subject: Re: Review Request 51397: HIVE-14536 Unit test code cleanup
From: Peter Vary <pvary@cloudera.com>
To: Zoltan Haindrich <kirk@rxd.hu>,
 Miklos Csanady <miklos.csanady@cloudera.com>,
 Siddharth Seth <sseth@apache.org>,
 Barna Zsombor Klara <zsombor.klara@cloudera.com>,
 Ashutosh Chauhan <hashutosh@apache.org>, Marta Kuczora <kuczoram@cloudera.com>,
 j.prasanth.j@gmail.com, Sergio Pena <sergio.pena@cloudera.com>,
 Sergey Shelukhin <sergey@hortonworks.com>,
 Gabor Szadovszky <gabor.szadovszky@cloudera.com>
Cc: Peter Vary <pvary@cloudera.com>, hive <dev@hive.apache.org>
Date: Fri, 26 Aug 2016 02:37:56 -0000
Message-ID: <20160826023756.2089.19111@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: Peter Vary <noreply@reviews.apache.org>
References: <20160825224639.2089.23367@reviews.apache.org>
In-Reply-To: <20160825224639.2089.23367@reviews.apache.org>
Reply-To: Peter Vary <pvary@cloudera.com>
archived-at: Fri, 26 Aug 2016 02:38:01 -0000

--===============5331892836499968643==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > 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

Hi Zoltan,

I know it is a big patch, and I appreciate your review very much, however I did not see the possibility to refactor this in smaller sets.

Thanks again, see my responses below,
Peter


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestBeeLineDriver.java, lines 37-39
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485268#file1485268line37>
> >
> >     this is not seems like a junit test 
> >     
> >     can you add the annotations + @Ignore("will be enabled soon ;)")

Thanks - I did this


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/qtest-accumulo/src/test/java/org/apache/hadoop/hive/cli/TestAccumuloCliDriver.java, lines 45-52
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485260#file1485260line45>
> >
> >     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..

I removed the possibility of the configuration changes intentionally. This left me with this version (no setters, just getters).
I think it would be more readable this way:

new CliConfig(
        "accumulo-handler/src/test/queries/positive",
        "accumulo-handler/src/test/results/positive",
        "itests/qtest/target/qfile-results/accumulo-handler/positive",
        null,
        QTestUtil.MiniClusterType.none,
        "",
        "q_test_init.sql",
        "q_test_cleanup.sql",
        true
);

But this does not conform the coding guidelines.

It would be even more descriptive with a bulder like code:
CliConfigBuilder.setQueryDir().setResultsDir()....build();

But I decided against it, because I do not think it is worth the effort.


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestCompareCliDriver.java, line 82
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485270#file1485270line82>
> >
> >     i think it would be better to move out "things" from QTestUtil instead of adding more...

That was the point, where I stopped my cleanup work, and decided to put that into another patch.
I agree with you, that the QTestUtil requires a cleanup too.

My view on the problem is, that the test cases are using simmilar code very often, which we do not want to duplicate. But, we do not want to use inheritance in testcases, which migth cause problems to us when trying to run them in parallel, so a these functions should be refactored to a helper class, like QTestUtil.

I think we should revisit this issue again when we clean up the QTestUtil classes.

What do you think?


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 42
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line42>
> >
> >     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...

I think the configuration is one of the most important part of the Test class - when there is a problem, it is most natural to look for it there. That is why I put it there.

The only public setters are for the CliConfig class are the excludeQuerySet, includeQuerySet, etc. which I think were public on the abstractcliconfig too.

I might be missing your point here. Could you help please?


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 70
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line70>
> >
> >     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

Thanks, changed the comment, could you please review?


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, lines 90-97
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line90>
> >
> >     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

The properties do not have to be in pom.xml, if someone specifies them in command line with -D argument, there will be set.

So if someone wants to regenerate the query results without overwriting the original versions, -DresultsDirectory=/tmp -Dtest.output.overwrite=true could be used, or if there is a new set of tests in a directory, then it could be used as well, without changing the code.

So I think they have a merit, and the they do not cause further problems.


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, lines 101-106
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line101>
> >
> >     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

Maybe I made an error but I think this code will set the metastoreType from the property, if it is set:

    String metastoreTypeString = System.getProperty("metastoreType");
    if (metastoreTypeString != null && metastoreTypeString.trim().length() !=0) {
      System.err.println("Property: metastoreType used as override with val: "
                             + metastoreTypeString);
      this.metastoreType = MetastoreType.valueOf(metastoreTypeString);

This property could be very usefull, if we want to test the HBase metastore with other testcases too.

Reopen please, if I made a mistake here, and missread the code above.

Thanks!


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 111
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line111>
> >
> >     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

Thanks for catching this - I remember changing to valueOf and removing the comment, but later I put it back, since I think we want backward compatiblity and with valueOf the string literals whould be changed. But I forget to put back the FIXME.

Added the FIXME, thanks


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, lines 118-122
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line118>
> >
> >     these defaults are not really usefull...i think an exception would be more appropriate in case of initScript of cleanupScript is missing

Default:
- hiveConfDir - it is currently used
- initScript - might not need to run any init script for several tests - if it resolves to a dir ("" means hiveRootDir), then no script will be run - It could be usefull, to run a specific test where no init tables are needed, since the initialization itself takes too long
- cleanupScript - same as initScript
- hadoopVersion - you get me here, I am not really comfortable with that one, but it should not cause any problems.


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 138
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line138>
> >
> >     try-with-resources

I am confortable with throwing an exception in test initialization, if there is no testconfiguration.properties, or there is any error. Since the only thing I could do is catch the exception and throw it again. I do not think converting it to an assertion failure is any better.


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 260
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line260>
> >
> >     i think it would be better to split this into 2 methods:
> >     above this line is getQueryFiles
> >     belove this is getTestParameters

You are right, that these two are distinct functions, but I think we do not want to expose or reuse getQueryFiles anyway, and a 5 line method does not really helps the readibility - added comment to help to understand the function 

If you still think we should split them, feel free to open an issue for it


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 275
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line275>
> >
> >     i think the earlier small notice about that a property override have happend was usefull in the test outputs

It will be printed here, in the method where the override happens:

  private String getProperty(String systemPropertyKey, String constructorValue, String defaultValue) {


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, lines 312-313
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line312>
> >
> >     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 ...

I have found some of them in another file:

hive/testutils/ptest2/conf/example-apache-trunk.properties

Plus it is loaded by several pom.xml-s:

      <plugin>
        <groupId>org.codehaus.mojo</groupId>
        <artifactId>properties-maven-plugin</artifactId>
        <version>1.0-alpha-2</version>
        <executions>
          <execution>
            <phase>initialize</phase>
            <goals>
              <goal>read-project-properties</goal>
            </goals>
            <configuration>
              <files>
                <file>${basedir}/../src/test/resources/testconfiguration.properties</file>
              </files>
            </configuration>
          </execution>
        </executions>
      </plugin>
      
So I decided to keep them as it is.


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CliConfig.java, line 348
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485288#file1485288line348>
> >
> >     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

The includeAll is not set by the user.
It is to resolve your previous FIXME comment:

  // FIXME: null value is treated differently on the other end..when those filter will be
  // moved...this may change
  
This means, the user used at least one inclusive query list (includeQuerySet, includeQuery), then this will be set to true, and only the selected files will be used, not the every file from the queryDirectory.

The comment is improved - hopefully :)


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, lines 219-237
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485300#file1485300line219>
> >
> >     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!

There is a function created by the q_test_init.sql script:

CREATE FUNCTION qtest_get_java_boolean AS 'org.apache.hadoop.hive.ql.udf.generic.GenericUDFTestGetJavaBoolean';

I handled it the same way, as the srcTables.
We should keep in mind this, when we refactor the QTestUtil classes


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, lines 2269-2283
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485300#file1485300line2269>
> >
> >     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?

In java 7 we could not use lamda-s, and I call this method to calculate the default value for the hadoopVersion property. See:

 this.hadoopVersion = getProperty("hadoop.version", null,
        QTestUtil.calculateHadoopVersionFromJar());
        
It is not acceptable here to throw an exception - most probably the version calculation would not fall back to the default value.

I think it might be possible to run several tests without a hadoop jar on the classpath, which means this calculation will fail.

If you think after these reasoning you still prefer to throw exceptions, feel free to reopen the issue - I will think this through too tonight :)


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 2283
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485300#file1485300line2283>
> >
> >     i think this catch block in its current form is just some unreachable code

Yeah.. you are right - I was too eager to catch everything :)
Removed


> On Aug. 25, 2016, 10:46 p.m., Zoltan Haindrich wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java, line 2263
> > <https://reviews.apache.org/r/51397/diff/1/?file=1485300#file1485300line2263>
> >
> >     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

renamed to getHadoopVersionFromJar


On Aug. 25, 2016, 10:46 p.m., Peter Vary wrote:
> > 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

I agree with you, and I see my mistake in adding more changes than absolutely needed to this refactor.

My reasoning was:
- Beeline - if there is a test, then it should be able to run, and TestBeeLineDriver was there after your patch.
- UDF - this was clearly a bug, which made me hard to test single tests - the udf was created, but not deleted, so the next time I had to clear everything before the test
- Merge negative, positive test runs - these were code duplication, and adding 2, 40 line length, simmilar methods to a class with this minute differences seemed like a bad idea to me
- IMHO these system properties does not increase the complexicity of the system too much. They are only the mirrors of existing properties, and only adding the possibility of overriding them in one single place. After overriding is done we can forget them, and use the calculated values as before.

I very much appreciate, that you review my code even though these difficulties!

I agree with you that the unused properties has to be removed, but I do not know which ones are used by the community members, and which ones are not. As for the new ones, I am sure I would like use most of them - maybe not the hadoop.version, but all the others. Moreover the new properties do not have to be set to run the tests - I have run them during my testing from the IDE, and did not cause any problem for me. If you have any problem feel free to share, so we can address them.

About the hadoop.version property - what does it supposed to do? As in your previous patch review we discussed that this might be overriden, in case it is needed.
What happens with the current code:
- If it is specified with a -D parameter, then that value is used
- Else the pom.xml ${hadoop.version} is used
- Else ... I think there is not really an else here - but then comes the getHadoopVersionFromJar
Do you think, the pom.xml hadoop.version is ok? Then we could remove getHadoopVersionFromJar, and every other stuff too. Is there a usecase to overwrite this? Should it be overwritten separately, than the pom.xml-s hadoop.version?
Any help is appreciated here.
Thanks!

The template like nature does not prevent the divergence of the tests - See: HIVE-14625 - The logging is added only for the CoreCliDriver, not the other ones. We should help the test developers by reviewing the changes, and with clear guidance in the development docs. If the testcase change is a common one, it should be done in the QTestUtil classes, which will effect every test case.

As for HIVE-14625, I had to incorporate those changes to my patch as well, since it modified files that are deleted by my patch. I added the logging to the QTestUtils, so it will be done for every test - I think this is the way we should go.

Thanks for your helpful comments, and review!

Peter


- Peter


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51397/#review146818
-----------------------------------------------------------


On Aug. 25, 2016, 11:24 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, 11:24 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 
>   pom.xml b05a2dc 
> 
> 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
> 
>


--===============5331892836499968643==--