You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary <pv...@cloudera.com> on 2017/10/05 11:08:23 UTC
Review Request 62782: HIVE-17706 Add a possibility to run the BeeLine
tests on the default database
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/
-----------------------------------------------------------
Review request for hive and Barna Zsombor Klara.
Bugs: HIVE-17706
https://issues.apache.org/jira/browse/HIVE-17706
Repository: hive-git
Description
-------
- Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
- Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
- Copied some masking from QTestUtil
- Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
- In the QFile made it possible to not call create/drop database command, if not needed.
I would value any advice where the cleanup logic should be kept.
- I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
- QFileBeeLineClient.beforeExecute
- QFileBeeLineClient.afterExecute
Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
- CoreBeeLineDriver.runCleanup
This is really cleanup stuff
Or we should leave as it is :)
Any comments are welcome.
Thanks,
Peter
Diffs
-----
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/CoreBeeLineDriver.java 9dfc253
itests/util/src/main/java/org/apache/hive/beeline/QFile.java e70ac38
itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
Diff: https://reviews.apache.org/r/62782/diff/1/
Testing
-------
Run the tests manually with, and without the test.beeline.run.parallel parameter
Thanks,
Peter Vary
Re: Review Request 62782: HIVE-17706 Add a possibility to run the
BeeLine tests on the default database
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/
-----------------------------------------------------------
(Updated Oct. 6, 2017, 10:43 a.m.)
Review request for hive and Barna Zsombor Klara.
Changes
-------
Rebased the patch
Bugs: HIVE-17706
https://issues.apache.org/jira/browse/HIVE-17706
Repository: hive-git
Description
-------
- Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
- Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
- Copied some masking from QTestUtil
- Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
- In the QFile made it possible to not call create/drop database command, if not needed.
I would value any advice where the cleanup logic should be kept.
- I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
- QFileBeeLineClient.beforeExecute
- QFileBeeLineClient.afterExecute
Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
- CoreBeeLineDriver.runCleanup
This is really cleanup stuff
Or we should leave as it is :)
Any comments are welcome.
Thanks,
Peter
Diffs (updated)
-----
itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java 1fdce17
itests/util/src/main/java/org/apache/hive/beeline/QFile.java 38b0d91
itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
Diff: https://reviews.apache.org/r/62782/diff/3/
Changes: https://reviews.apache.org/r/62782/diff/2-3/
Testing
-------
Run the tests manually with, and without the test.beeline.run.parallel parameter
Thanks,
Peter Vary
Re: Review Request 62782: HIVE-17706 Add a possibility to run the
BeeLine tests on the default database
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/#review187240
-----------------------------------------------------------
Ship it!
Ship It!
- Barna Zsombor Klara
On Oct. 5, 2017, 4:39 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62782/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 4:39 p.m.)
>
>
> Review request for hive and Barna Zsombor Klara.
>
>
> Bugs: HIVE-17706
> https://issues.apache.org/jira/browse/HIVE-17706
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
> - Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
> - Copied some masking from QTestUtil
> - Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
> - In the QFile made it possible to not call create/drop database command, if not needed.
>
> I would value any advice where the cleanup logic should be kept.
> - I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
>
> Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
> - QFileBeeLineClient.beforeExecute
> - QFileBeeLineClient.afterExecute
> Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
>
> Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
> - CoreBeeLineDriver.runCleanup
> This is really cleanup stuff
>
> Or we should leave as it is :)
>
> Any comments are welcome.
> Thanks,
> Peter
>
>
> Diffs
> -----
>
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java 9dfc253
> itests/util/src/main/java/org/apache/hive/beeline/QFile.java e70ac38
> itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
>
>
> Diff: https://reviews.apache.org/r/62782/diff/2/
>
>
> Testing
> -------
>
> Run the tests manually with, and without the test.beeline.run.parallel parameter
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 62782: HIVE-17706 Add a possibility to run the
BeeLine tests on the default database
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/
-----------------------------------------------------------
(Updated Oct. 5, 2017, 4:39 p.m.)
Review request for hive and Barna Zsombor Klara.
Changes
-------
- renamed run.parallel to shared.database
- moved cleanup to QFileBeeLineClient
Bugs: HIVE-17706
https://issues.apache.org/jira/browse/HIVE-17706
Repository: hive-git
Description
-------
- Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
- Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
- Copied some masking from QTestUtil
- Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
- In the QFile made it possible to not call create/drop database command, if not needed.
I would value any advice where the cleanup logic should be kept.
- I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
- QFileBeeLineClient.beforeExecute
- QFileBeeLineClient.afterExecute
Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
- CoreBeeLineDriver.runCleanup
This is really cleanup stuff
Or we should leave as it is :)
Any comments are welcome.
Thanks,
Peter
Diffs (updated)
-----
itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java 9dfc253
itests/util/src/main/java/org/apache/hive/beeline/QFile.java e70ac38
itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
Diff: https://reviews.apache.org/r/62782/diff/2/
Changes: https://reviews.apache.org/r/62782/diff/1-2/
Testing
-------
Run the tests manually with, and without the test.beeline.run.parallel parameter
Thanks,
Peter Vary
Re: Review Request 62782: HIVE-17706 Add a possibility to run the
BeeLine tests on the default database
Posted by Peter Vary <pv...@cloudera.com>.
> On Oct. 5, 2017, 11:58 a.m., Barna Zsombor Klara wrote:
> > itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java
> > Line 119 (original), 122 (patched)
> > <https://reviews.apache.org/r/62782/diff/1/?file=1846490#file1846490line122>
> >
> > This may be OK, but the naming confuses me. The method is called setTestSpecificDatabase so I would expect a database name to test, yet we feed it something called parallel?
Changed the naming
> On Oct. 5, 2017, 11:58 a.m., Barna Zsombor Klara wrote:
> > itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java
> > Lines 80 (patched)
> > <https://reviews.apache.org/r/62782/diff/1/?file=1846492#file1846492line80>
> >
> > Please add javadoc for public APIs.
Moved to QFileBeeLineClient, and made it private :D
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/#review187168
-----------------------------------------------------------
On Oct. 5, 2017, 4:39 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62782/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 4:39 p.m.)
>
>
> Review request for hive and Barna Zsombor Klara.
>
>
> Bugs: HIVE-17706
> https://issues.apache.org/jira/browse/HIVE-17706
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
> - Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
> - Copied some masking from QTestUtil
> - Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
> - In the QFile made it possible to not call create/drop database command, if not needed.
>
> I would value any advice where the cleanup logic should be kept.
> - I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
>
> Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
> - QFileBeeLineClient.beforeExecute
> - QFileBeeLineClient.afterExecute
> Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
>
> Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
> - CoreBeeLineDriver.runCleanup
> This is really cleanup stuff
>
> Or we should leave as it is :)
>
> Any comments are welcome.
> Thanks,
> Peter
>
>
> Diffs
> -----
>
> itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java 9dfc253
> itests/util/src/main/java/org/apache/hive/beeline/QFile.java e70ac38
> itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
>
>
> Diff: https://reviews.apache.org/r/62782/diff/2/
>
>
> Testing
> -------
>
> Run the tests manually with, and without the test.beeline.run.parallel parameter
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 62782: HIVE-17706 Add a possibility to run the
BeeLine tests on the default database
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62782/#review187168
-----------------------------------------------------------
Thank you for the patch Peter, looking good.
I don't have a clear preference on where the cleanup should go, but I would like it in one class. CoreBeeLineDriver or QFileBeeLineClient doesn't matter to me much, whichever is easier to refactor.
itests/util/src/main/java/org/apache/hadoop/hive/cli/control/CoreBeeLineDriver.java
Line 119 (original), 122 (patched)
<https://reviews.apache.org/r/62782/#comment264079>
This may be OK, but the naming confuses me. The method is called setTestSpecificDatabase so I would expect a database name to test, yet we feed it something called parallel?
itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java
Lines 80 (patched)
<https://reviews.apache.org/r/62782/#comment264082>
Please add javadoc for public APIs.
- Barna Zsombor Klara
On Oct. 5, 2017, 11:08 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62782/
> -----------------------------------------------------------
>
> (Updated Oct. 5, 2017, 11:08 a.m.)
>
>
> Review request for hive and Barna Zsombor Klara.
>
>
> Bugs: HIVE-17706
> https://issues.apache.org/jira/browse/HIVE-17706
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> - Added a cleanup method which removed unkonwn databases, tables and view. This makes it is possible to run the tests sequentially using 'default' database.
> - Renamed the test.rewrite.source.tables parameter to test.beeline.run.parallel so it makes more sense.
> - Copied some masking from QTestUtil
> - Enhanced the table name rewriting regexp a little, so mixed case tablenames are kept as it is
> - In the QFile made it possible to not call create/drop database command, if not needed.
>
> I would value any advice where the cleanup logic should be kept.
> - I this solution when the tests are parallel then the cleanup is in QFileBeeLineClient (create/drop database), when the tests are sequential then the cleanup is in the CoreBeeLineDriver.
>
> Would it be a good idea to move every cleanup related stuff to CoreBeeLineDriver? Like:
> - QFileBeeLineClient.beforeExecute
> - QFileBeeLineClient.afterExecute
> Both of these are need QFile specific info, and an existing BeeLineClient, which we currently do not have in hand in the CoreBeeLineDriver, and more refactoring is needed
>
> Or would it be a good idea to move every cleanup related stuff to QFileBeeLineClient? Like:
> - CoreBeeLineDriver.runCleanup
> This is really cleanup stuff
>
> Or we should leave as it is :)
>
> Any comments are welcome.
> Thanks,
> Peter
>
>
> Diffs
> -----
>
> 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/CoreBeeLineDriver.java 9dfc253
> itests/util/src/main/java/org/apache/hive/beeline/QFile.java e70ac38
> itests/util/src/main/java/org/apache/hive/beeline/QFileBeeLineClient.java 2f91834
>
>
> Diff: https://reviews.apache.org/r/62782/diff/1/
>
>
> Testing
> -------
>
> Run the tests manually with, and without the test.beeline.run.parallel parameter
>
>
> Thanks,
>
> Peter Vary
>
>