> On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > Hello Peter! > > > > It's great to see that the qfile has a whole representative class now and > > the client aspect is now more separated from it...it even makes reading it > > more easier! > > I've found some minor issues - and added some notes here and there :) > > > > cheers
Thanks for the review Zoltan! Again helpfull comments! > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > > Lines 109 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659294#file1659294line116> > > > > it would be great to use java.io.File (or Path or ?) api a bit > > more...and get rid of the many string concatenations... Good idea, thanks. Done. > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > > Line 110 (original), 142 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659294#file1659294line160> > > > > if the execution fails.. (parser error)..there is a very > > uncommunicative exception saying null...could you add some description to > > this assertion: something like 'execution failed' or something - it would > > be good to show the user that the test system worked as expected - but the > > qfile have failed. Done. This time only with a simple message. > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > > Line 121 (original), 158 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659294#file1659294line181> > > > > it would be great to give some more info about the failure...not sure > > what info is available at this point - but I really miss the pointer to > > 'hive.log'... > > (possibly follow-up) Will check what Zsombor planned for the Qtests in HIVE-15616. Created a followup jira, to do provide more informative error messages and test output: HIVE-16152 > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java > > Lines 165 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659294#file1659294line192> > > > > i think this could be done with try-with-resources Agreed, done > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 43-49 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659295#file1659295line43> > > > > it would be great to have a bunch of File object references > > here...these strings always cause troubles - here and there (possibly in a > > followup) Done it here. Refactored to use File-s where possible. > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 100-103 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659295#file1659295line100> > > > > instead of copyToDirectory...I would recommend to use the expected - I > > know they should be the same... (or possibly there is some hidden magic > > which I'm not aware of :) The original hidden magic was about creating the resultsDirectory folder, if that folder or any of the parents are not exists, then copyFileToDirectory(..,true) created the missing directories. I do not think we need that here, so removed > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 105 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659295#file1659295line105> > > > > I think it would be better to throw this execption out...the user > > expects it to be overwritten - if this is the only thing that fails - it > > may cover-up to a non-updateable file like dir and file owned by root by a > > mistake... Agreed, done > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java > > Lines 266 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659295#file1659295line266> > > > > I don't really see why staticfilterset is needed...and it's field - it > > seems to me that they are used at the same time... I have created two groups of filters: - One which is same for every tests - One which is test specific This way we have to create fewer objects when we run multiple tests. Added comments to clarify the difference > On March 8, 2017, 4:43 p.m., Zoltan Haindrich wrote: > > itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java > > Lines 70 (patched) > > <https://reviews.apache.org/r/57343/diff/2/?file=1659296#file1659296line70> > > > > i don't think this should be asserted...if you run your jvm without -ea > > this method is not even invoked Surefire plugin set this to true by default, and we do not change it in our pom. Doublechecked with an actual failure and with our current settings it throws the AssertionError as expected, but I think I will change this in HIVE-16152 anyway to provide a more helpfull message :) - Peter ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57343/#review168302 ----------------------------------------------------------- On March 9, 2017, 10:05 a.m., Peter Vary wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57343/ > ----------------------------------------------------------- > > (Updated March 9, 2017, 10:05 a.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 d99483e > itests/src/test/resources/testconfiguration.properties 2a7627a > > 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/3/ > > > 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 > >
