> On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > >
Thanks for the review Vihang! > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > > Lines 44-45 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658136#file1658136line44> > > > > Nit : I see that necessary imports are already there (or could be > > added) is there a reason to declare it this way? Done > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 71 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658137#file1658137line71> > > > > Nit : Spellcheck - getExpectedOutputFile() Done > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 195-197 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658137#file1658137line195> > > > > Is this pre-existing code? Is there a way to make this more robust? A > > change in this log message in the source code will fail all the BeeLine > > tests. It is a pre-existing code, but neverless your comment is valid. I have created a jira for this (HIVE-16146), and we can discuss the possibilities there. > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 259-264 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658137#file1658137line259> > > > > Use File.separator instead of "/" to make it OS agnostic Thanks for pointing this out. Done. > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > > Lines 71-72 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658138#file1658138line71> > > > > Curious to understand why do we need to create one database for each > > qFile? Also, may be a good idea to refactor these lines into a > > beforeExecute() method This could help us archive multiple things: - Better separation of the tests - easier to drop a whole database at the end of the tests - My final goal would be to create 1 MiniHS2 server and run multiple beeline test against them parallel. In this case the separation is a must. What do you think? Refactored the code to beforeExecute() > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > > Lines 80-83 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658138#file1658138line80> > > > > Can we refactor this to a afterExecute() method Refactored to afterExecute() > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > > Lines 94-95 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658138#file1658138line94> > > > > Do we need this in a finally block? When running multiple tests I prefer to cleanly close the BeeLine connection > On March 7, 2017, 6:13 p.m., Vihang Karajgaonkar wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/package-info.java > > Lines 22 (patched) > > <https://reviews.apache.org/r/57343/diff/1/?file=1658139#file1658139line22> > > > > Not sure I understand this file.. Our current checkstyle configuration contains this: <!-- Checks that a package.html file exists for each package. --> <module name="JavadocPackage"/> So it checks if there is a pacakge-info for every new package. Since I have created this package I created the package-info.java file which is used for generating the package.html when generating the javadoc. As an owner of HIVE-15051, I feel obliged to write code without checkstyle errors :D - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57343/#review168162 ----------------------------------------------------------- On March 8, 2017, 3:03 p.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57343/ > ----------------------------------------------------------- > > (Updated March 8, 2017, 3:03 p.m.) > > > Review request for hive, Zoltan Haindrich, Naveen Gangam, Sergio Pena, Vihang > Karajgaonkar, and Barna Zsombor Klara. > > > Bugs: HIVE-16127 > https://issues.apache.org/jira/browse/HIVE-16127 > > > Repository: hive-git > > > Description > ------- > > Refactored the QFileClient: > - Moved to itest/util > - Separated QFile specific code parts (file path, and filtering) > - Separated BeeLineClient specific code (execution of the queries, commands) > - Created factories for QFile, and Client, so after initialization they can > be reused during multiple tests > - Separated init script run from actual test run, so multiple tests could be > run against a single MiniHS2 instance > - Removed unused property > > > Diffs > ----- > > beeline/src/java/org/apache/hive/beeline/util/QFileClient.java d306b7f > itests/src/test/resources/testconfiguration.properties b01ebd8 > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > aba1fde > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > PRE-CREATION > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > PRE-CREATION > itests/util/src/main/java/org/apache/hive/beeline/qfile/package-info.java > PRE-CREATION > ql/src/test/results/clientpositive/beeline/drop_with_concurrency.q.out > PRE-CREATION > > > Diff: https://reviews.apache.org/r/57343/diff/2/ > > > Testing > ------- > > Used the previosly generated test to validate that the result is not changed. > Added one more test to check the separation is done correctly. > > > Thanks, > > Peter Vary > >
