----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57343/#review168302 -----------------------------------------------------------
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 itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java Lines 109 (patched) <https://reviews.apache.org/r/57343/#comment240505> it would be great to use java.io.File (or Path or ?) api a bit more...and get rid of the many string concatenations... itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java Line 110 (original), 142 (patched) <https://reviews.apache.org/r/57343/#comment240515> 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. itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java Line 121 (original), 158 (patched) <https://reviews.apache.org/r/57343/#comment240514> 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) itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java Lines 165 (patched) <https://reviews.apache.org/r/57343/#comment240516> i think this could be done with try-with-resources itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java Lines 43-49 (patched) <https://reviews.apache.org/r/57343/#comment240512> 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) itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java Lines 100-103 (patched) <https://reviews.apache.org/r/57343/#comment240517> 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 :) itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java Lines 105 (patched) <https://reviews.apache.org/r/57343/#comment240518> 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... itests/util/src/main/java/org/apache/hive/beeline/qfile/QFile.java Lines 266 (patched) <https://reviews.apache.org/r/57343/#comment240519> 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... itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java Lines 70 (patched) <https://reviews.apache.org/r/57343/#comment240520> i don't think this should be asserted...if you run your jvm without -ea this method is not even invoked itests/util/src/main/java/org/apache/hive/beeline/qfile/QFileBeeLineClient.java Lines 82 (patched) <https://reviews.apache.org/r/57343/#comment240521> same assert related thing here - Zoltan Haindrich 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 > >
