You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Zoltan Haindrich <ki...@rxd.hu> on 2016/08/03 20:56:57 UTC
Review Request 50768: HIVE-14415 Upgrade qtest execution framework to
junit4 - TestPerfCliDriver
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50768/
-----------------------------------------------------------
Review request for hive.
Bugs: HIVE-14415
https://issues.apache.org/jira/browse/HIVE-14415
Repository: hive-git
Description
-------
i'm using mostly the same code as QTestGenTask...I even made some of it's filter accessible for now.
this patch also contains the upgrade of TestPerfCliDriver to junit4...which may make it a bit less readable.
other changes:
* HIVE_ROOT is detected by the existence of some specific directories
* hadoop_version is parsed from a jar instead of using some external property
Diffs
-----
ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b
itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java PRE-CREATION
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b3cf6da56eddc5000515e5ebd34989e64b1c0010
ql/src/test/templates/TestPerfCliDriver.vm d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a
Diff: https://reviews.apache.org/r/50768/diff/
Testing
-------
i've already run it "alone" using maven - all the 60 cases have passed.
Thanks,
Zoltan Haindrich
Re: Review Request 50768: HIVE-14415 Upgrade qtest execution
framework to junit4 - TestPerfCliDriver
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50768/#review144773
-----------------------------------------------------------
Thanks for simpilying this, dropping the velocity/ant dependency is really appreciated.
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 191)
<https://reviews.apache.org/r/50768/#comment210886>
Do we need a static initializer for srcTables?
I can't find it being referenced from a static context.
Couldn't we have an instance variable initialized in the constructor instead of having it rebuilt every time?
- Barna Zsombor Klara
On Aug. 3, 2016, 8:56 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50768/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 8:56 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-14415
> https://issues.apache.org/jira/browse/HIVE-14415
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> i'm using mostly the same code as QTestGenTask...I even made some of it's filter accessible for now.
>
> this patch also contains the upgrade of TestPerfCliDriver to junit4...which may make it a bit less readable.
>
> other changes:
>
> * HIVE_ROOT is detected by the existence of some specific directories
> * hadoop_version is parsed from a jar instead of using some external property
>
>
> Diffs
> -----
>
> ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b
> itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java PRE-CREATION
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b3cf6da56eddc5000515e5ebd34989e64b1c0010
> ql/src/test/templates/TestPerfCliDriver.vm d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a
>
> Diff: https://reviews.apache.org/r/50768/diff/
>
>
> Testing
> -------
>
> i've already run it "alone" using maven - all the 60 cases have passed.
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 50768: HIVE-14415 Upgrade qtest execution
framework to junit4 - TestPerfCliDriver
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50768/#review144812
-----------------------------------------------------------
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 46 - 47)
<https://reviews.apache.org/r/50768/#comment210946>
honestly: it might happen...but qtestgentask had it...so it might get used in the next cli class...i would like to postone changes like this after all the vm files are removed - this is almost the same logic as what qtestgentask uses internally...so comparing it might be easier this way - after everythings good; think this whole getQueryFiles() will be deconstructed.
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 113)
<https://reviews.apache.org/r/50768/#comment210933>
honestly: it might happen...but qtestgentask had it...so it might get used in the next cli class...i would like to postone changes like this after all the vm files are removed - this is almost the same logic as what qtestgentask uses internally...so comparing it might be easier this way - after everythings good; think this whole getQueryFiles() will be deconstructed.
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 125)
<https://reviews.apache.org/r/50768/#comment210947>
this will be changed later
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 128 - 135)
<https://reviews.apache.org/r/50768/#comment210942>
sure, but later
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 138)
<https://reviews.apache.org/r/50768/#comment210943>
of course, and possibly use better ones.
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 150 - 164)
<https://reviews.apache.org/r/50768/#comment210945>
i totally agree..i've seen it; but this have came from qtestgentask...i've removed a few unnecessary things already
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 170)
<https://reviews.apache.org/r/50768/#comment210921>
i just wanted to keep the backward compatibility...the original qtesttask have passed hadoop.version
i'm not yet sure that this deep-dive-into-resources will work in every cases...just in case it's needed later
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 186)
<https://reviews.apache.org/r/50768/#comment210922>
great idea! ;)
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 191)
<https://reviews.apache.org/r/50768/#comment210930>
this static class initializer have caused some exceptions for me...i just wanted to get rid of the load time problems...
but yes! this it's not used anywhere else...=> private field initialized using constructor
cool!
ql/src/test/templates/TestPerfCliDriver.vm (line 56)
<https://reviews.apache.org/r/50768/#comment210925>
yeah...well I've just wanted to keep the 'original' clidriver manner intact as much as possible ;)
import static also works ;)
ql/src/test/templates/TestPerfCliDriver.vm (line 58)
<https://reviews.apache.org/r/50768/#comment210928>
it's the testcase name; apparently its unused ;)
ql/src/test/templates/TestPerfCliDriver.vm (line 163)
<https://reviews.apache.org/r/50768/#comment210927>
well..yes ;)
- Zoltan Haindrich
On Aug. 4, 2016, 9:25 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50768/
> -----------------------------------------------------------
>
> (Updated Aug. 4, 2016, 9:25 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-14415
> https://issues.apache.org/jira/browse/HIVE-14415
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> i'm using mostly the same code as QTestGenTask...I even made some of it's filter accessible for now.
>
> this patch also contains the upgrade of TestPerfCliDriver to junit4...which may make it a bit less readable.
>
> other changes:
>
> * HIVE_ROOT is detected by the existence of some specific directories
> * hadoop_version is parsed from a jar instead of using some external property
>
>
> Diffs
> -----
>
> ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b
> itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java PRE-CREATION
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestPerfCliDriver.java PRE-CREATION
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b3cf6da56eddc5000515e5ebd34989e64b1c0010
> ql/src/test/templates/TestPerfCliDriver.vm d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a
>
> Diff: https://reviews.apache.org/r/50768/diff/
>
>
> Testing
> -------
>
> i've already run it "alone" using maven - all the 60 cases have passed.
>
>
> Thanks,
>
> Zoltan Haindrich
>
>
Re: Review Request 50768: HIVE-14415 Upgrade qtest execution
framework to junit4 - TestPerfCliDriver
Posted by Zoltan Haindrich <ki...@rxd.hu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50768/
-----------------------------------------------------------
(Updated Aug. 4, 2016, 9:25 p.m.)
Review request for hive.
Changes
-------
updated patch
Bugs: HIVE-14415
https://issues.apache.org/jira/browse/HIVE-14415
Repository: hive-git
Description
-------
i'm using mostly the same code as QTestGenTask...I even made some of it's filter accessible for now.
this patch also contains the upgrade of TestPerfCliDriver to junit4...which may make it a bit less readable.
other changes:
* HIVE_ROOT is detected by the existence of some specific directories
* hadoop_version is parsed from a jar instead of using some external property
Diffs (updated)
-----
ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b
itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java PRE-CREATION
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/TestPerfCliDriver.java PRE-CREATION
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b3cf6da56eddc5000515e5ebd34989e64b1c0010
ql/src/test/templates/TestPerfCliDriver.vm d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a
Diff: https://reviews.apache.org/r/50768/diff/
Testing
-------
i've already run it "alone" using maven - all the 60 cases have passed.
Thanks,
Zoltan Haindrich
Re: Review Request 50768: HIVE-14415 Upgrade qtest execution
framework to junit4 - TestPerfCliDriver
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/50768/#review144743
-----------------------------------------------------------
Great stuff overall, thanks for the patch!
Feel free to drop any of my issues, if you think so. I am new to hive, and just learning, so I am sure I made some mistakes during the review.
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 46 - 47)
<https://reviews.apache.org/r/50768/#comment210833>
I think you have planned to do something with these (includeQueryFile, excludeQueryFile) - currently they are not set
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 105)
<https://reviews.apache.org/r/50768/#comment210839>
nit extra line
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 113)
<https://reviews.apache.org/r/50768/#comment210840>
I think includeQueryFile is always empty.
Not initialized yet.
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 125)
<https://reviews.apache.org/r/50768/#comment210841>
nit "Setup" maybe?
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 128 - 135)
<https://reviews.apache.org/r/50768/#comment210842>
nit Should'n we create a filterlike parser for this? Would stroke my desire for simmetricity :)
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 138)
<https://reviews.apache.org/r/50768/#comment210843>
nit Later we should move these filters from the ant package
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (lines 150 - 164)
<https://reviews.apache.org/r/50768/#comment210844>
I would put these somewhere in the begining of the execution, like into the constructor.
The HIVE_ROOT is already used in this method before, so it is better to be existing at this moment
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 170)
<https://reviews.apache.org/r/50768/#comment210845>
Who would set this version number?
itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java (line 186)
<https://reviews.apache.org/r/50768/#comment210846>
nit You might offer to set the hadoop.version manually in the exception, like before
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 191)
<https://reviews.apache.org/r/50768/#comment210847>
nit space between ){
itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java (line 192)
<https://reviews.apache.org/r/50768/#comment210849>
nit Extra space maybe in front of the line?
ql/src/test/templates/TestPerfCliDriver.vm (line 56)
<https://reviews.apache.org/r/50768/#comment210851>
nit Maybe it is just me, but I would use a getter here
ql/src/test/templates/TestPerfCliDriver.vm (line 58)
<https://reviews.apache.org/r/50768/#comment210854>
What is this name is used for?
ql/src/test/templates/TestPerfCliDriver.vm (line 82)
<https://reviews.apache.org/r/50768/#comment210852>
nit extra line
ql/src/test/templates/TestPerfCliDriver.vm (line 84)
<https://reviews.apache.org/r/50768/#comment210853>
nit extra line
ql/src/test/templates/TestPerfCliDriver.vm (line 163)
<https://reviews.apache.org/r/50768/#comment210856>
We could remove tname here? Won't we?
Thanks again,
Peter
- Peter Vary
On Aug. 3, 2016, 8:56 p.m., Zoltan Haindrich wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50768/
> -----------------------------------------------------------
>
> (Updated Aug. 3, 2016, 8:56 p.m.)
>
>
> Review request for hive.
>
>
> Bugs: HIVE-14415
> https://issues.apache.org/jira/browse/HIVE-14415
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> i'm using mostly the same code as QTestGenTask...I even made some of it's filter accessible for now.
>
> this patch also contains the upgrade of TestPerfCliDriver to junit4...which may make it a bit less readable.
>
> other changes:
>
> * HIVE_ROOT is detected by the existence of some specific directories
> * hadoop_version is parsed from a jar instead of using some external property
>
>
> Diffs
> -----
>
> ant/src/org/apache/hadoop/hive/ant/QTestGenTask.java f372d7cb937d91c10d3dff0e4c51e0849c5e3c9b
> itests/qtest/pom.xml 17968e69559a16a1971f6028ea3073ab693a6678
> itests/qtest/src/test/java/org/apache/hadoop/hive/cli/AbstractCliConfig.java PRE-CREATION
> itests/util/src/main/java/org/apache/hadoop/hive/ql/QTestUtil.java b3cf6da56eddc5000515e5ebd34989e64b1c0010
> ql/src/test/templates/TestPerfCliDriver.vm d2946cb4f9a7e3e9f8b3cc14a75802a483e7f95a
>
> Diff: https://reviews.apache.org/r/50768/diff/
>
>
> Testing
> -------
>
> i've already run it "alone" using maven - all the 60 cases have passed.
>
>
> Thanks,
>
> Zoltan Haindrich
>
>