You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@impala.apache.org by "Anonymous Coward (Code Review)" <ge...@cloudera.org> on 2016/08/17 01:17:43 UTC

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator 

stakiar@cloudera.com has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4011

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................

IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Re-enabling Hive as a target DB for the data-generator and discrepancy checker.

* Added hive cli options back in (removed in 9608085d0eb6da233c877830b404ad748f45e20d)
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded file under IMPALA_HOME

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
4 files changed, 51 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 816:   def cursor(self):
> What's the point of overriding this method? It does the same thing as the m
The parent class uses ImpalaConnection, this method uses HiveConnection. It's necessary so that the Hive cursor connects to Hive rather than Impala.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/3/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS3, Line 56: DEFAULT_HIVE_PORT = 10000
This should be reverted back to 11050 to match what's already in use in the Impala dev environment (for good or ill).


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has submitted this change and it was merged.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Reviewed-on: http://gerrit.cloudera.org:8080/4011
Reviewed-by: Taras Bobrovytsky <tb...@cloudera.com>
Tested-by: Taras Bobrovytsky <tb...@cloudera.com>
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 121 insertions(+), 24 deletions(-)

Approvals:
  Taras Bobrovytsky: Looks good to me, approved; Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 7:

> > The hive-default.xml file under
 > > fe/src/test/resources/ is not a valid XML file. It was missing
 > some
 > > element terminators. Once I fixed those the tests starting
 > working.
 > > I have updated the patch.
 > 
 > Thanks, can you update the commit message as well?

Updated

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 4:

I'd like you to run the small suite of cluster tests, but you can't yet until you rebase on top of https://gerrit.cloudera.org/#/c/4404/ I realize there aren't that many, and they may not find anything, but it's a habit we need to start. Thanks.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4011

to look at the new patch set (#9).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 92 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/9
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................


Patch Set 1:

> (1 comment)

Thanks for the review! Will work on addressing the changes. Currently working on getting a test environment set up for the RQG, having some trouble so it may be another few days before I can get this review updated.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 13: Code-Review+2 Verified+1

Looks good to me, I'll submit this patch.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 13
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 5:

> Uploaded patch set 5: Patch Set 4 was rebased.

@Michael, not sure what happened there, but I just pushed the rebased version.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has uploaded a new patch set (#6).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
4 files changed, 66 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/6
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 4:

Sahil can you please push the rebased commit to Gerrit? If you kept the Change-Id in the commit log (as you should have) then the rebased commit will be patch set 5 of this review.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 7:

> This needs a committer's look.

@Michael, do we know of a committer that has time to look at this patch?

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 4:

> > I'd like you to run the small suite of cluster tests, but you
 > can't
 > > yet until you rebase on top of https://gerrit.cloudera.org/#/c/4404/
 > > I realize there aren't that many, and they may not find anything,
 > > but it's a habit we need to start. Thanks.
 > 
 > Will do, looks like your patch just got merged. I will rebase my
 > patch and then run the tests. Will update this review once I'm
 > done.

@Michael, rebased this patch and ran all the units tests (test_cluster.py and test_query_objects.py). All tests passed.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 7: Code-Review+1

This needs a committer's look.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4011/7/tests/comparison/cluster.py
File tests/comparison/cluster.py:

Line 100:     if default is None:
I think it's a little confusing to check if default is None here.

How about:

result = self._hadoop_configs.get(key, default)
if result is None:
  raise ...
return result


Line 185:     other_conf_dir = os.environ["HIVE_CONF_DIR"]
I'm not sure that modifying the minicluster here is the right thing to do. It represents the minicluster environment in the Impala repository. How are you running, the Hive tests? Do you use this environment or you run against a cluster?


http://gerrit.cloudera.org:8080/#/c/4011/7/tests/comparison/data_generator.py
File tests/comparison/data_generator.py:

Line 92:     self.db_name = None
How about adding a variable here called, for example, db_engine that can be either set to Hive or Impala? Then we won't have to worry about passing is_hive to populate_db every time.


Line 153:         cursor.compute_stats()
We don't need to compute stats if we are using hive?


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4011/7/tests/comparison/cluster.py
File tests/comparison/cluster.py:

Line 100:     if result is None:
> I think it's a little confusing to check if default is None here.
Done


Line 185:     for file_name in ["hive-site.xml"]:
> I'm not sure that modifying the minicluster here is the right thing to do. 
I run the Hive tests against a local instance of Hive, so setting this to HIVE_CONF_DIR makes development for me much. I think it will also be necessary to run the RQG against Hive in general as Hive-on-RQG should be able to accept a custom hive-default.xml file.

I've tested this change against Impala (ran the data_generator, discpreancy_checker, and Leopard) and it works fine. There were a few failures because now the the code uses fe/src/test/resources/hive-default.xml which had some invalid xml clauses.

If you think this will cause problems for Impala, I can revert it. However, in that case I would like to create a new Cluster object that can accept generic hive-default.xml files.


http://gerrit.cloudera.org:8080/#/c/4011/7/tests/comparison/data_generator.py
File tests/comparison/data_generator.py:

Line 92:     self.cluster = None
> How about adding a variable here called, for example, db_engine that can be
Done


Line 153:       with self.cluster.impala.cursor(db_name=self.db_name) as cursor:
> We don't need to compute stats if we are using hive?
Thats a good point, updated the patch to do this.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 816:   def cursor(self):
> The parent class uses ImpalaConnection, this method uses HiveConnection. It
_CURSOR_CLASS controls if we connect to Hive instead of Impala. It should be removed. (Try removing it and make sure it still works).


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 11:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/cluster.py
File tests/comparison/cluster.py:

Line 211: class MiniHiveCluster(MiniCluster):
Nice.


http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 816:   def cursor(self):
What's the point of overriding this method? It does the same thing as the method in the parent class?


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................


Patch Set 2:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4011/1//COMMIT_MSG
Commit Message:

PS1, Line 9: Changes:
> Remove; redundant with line 7.
Fixed


PS1, Line 11: * Added hive cli options back in (removed in commit "Stress test: Various changes")
> Even better, don't refer to hashes: they can and will change across branche
Removed commit hash and replaced it with the commit message


PS1, Line 14: * Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
> Please wrap at 90 chars.
Wrapped


Line 15: file under IMPALA_HOME
> Since unfortunately there are no unit tests for the query generator, it's i
Added details about how I tested the changes


http://gerrit.cloudera.org:8080/#/c/4011/1/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS1, Line 76:   def _load_hadoop_config(self)
> It seems like this should be a private method. You haven't actually changed
Yes, it can be private, updated.


PS1, Line 96:     """
> Maybe work the logic to handle this into the method above, so two first-cla
Collapsed these into one method get_hadoop_config(self, key, default=None):


Line 325: 
> Long line; please wrap at 90.
Wrapped


PS1, Line 422: 
> Long line; please wrap at 90.
Wrapped


http://gerrit.cloudera.org:8080/#/c/4011/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 799
> Revert this change. flake8 likes 2 lines between classes anyway.
Reverted


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4011

to look at the new patch set (#12).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 121 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/12
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has uploaded a new patch set (#4).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
3 files changed, 61 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 11:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 816:   def cursor(self):
> _CURSOR_CLASS controls if we connect to Hive instead of Impala. It should b
I meant this method should be removed.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 6:

> The hive-default.xml file under
 > fe/src/test/resources/ is not a valid XML file. It was missing some
 > element terminators. Once I fixed those the tests starting working.
 > I have updated the patch.

Thanks, can you update the commit message as well?

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................


Patch Set 1:

(9 comments)

Thanks for the patch! Please add Michael Brown, David Knupp, and Taras Bobrovytsky as reviewers.

http://gerrit.cloudera.org:8080/#/c/4011/1//COMMIT_MSG
Commit Message:

PS1, Line 9: Re-enabling Hive as a target DB for the data-generator and discrepancy checker.
Remove; redundant with line 7.


PS1, Line 11: * Added hive cli options back in (removed in 9608085d0eb6da233c877830b404ad748f45e20d)
Refer to commit hash f288867833b6f2953ef570217b953c9d523435b4 here, since that's the hash along the Impala-ASF master branch, and this patch is destined for the same. Check my work to make sure you agree. :)


PS1, Line 14: * Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded file under IMPALA_HOME
Please wrap at 90 chars.


Line 15: 
Since unfortunately there are no unit tests for the query generator, it's important for you to show that you tested your patch manually. Can you also talk about that in the commit message? Please talk about

1. Testing new functionality (for example, one or more sample commands executed, expected and confirmed result)

2. Regression testing (same as above, and also checking other entry points that may hit your touched codepaths)


http://gerrit.cloudera.org:8080/#/c/4011/1/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS1, Line 76:   def load_hadoop_config(self):
It seems like this should be a private method. You haven't actually changed the interface to Cluster, just abstracted out the loading step. Put another way, callers shouldn't typically have to call this method after instantiation but before getting a value from the config, right?


PS1, Line 96:   def get_hadoop_config_with_default(self, key, default):
Maybe work the logic to handle this into the method above, so two first-class methods are not needed?


Line 325:     endpoint = self.cluster.get_hadoop_config_with_default("dfs.namenode.http-address", "0.0.0.0:50070")
Long line; please wrap at 90.


PS1, Line 422:       self._warehouse_dir = urlparse(self.cluster.get_hadoop_config("hive.metastore.warehouse.dir")).path
Long line; please wrap at 90.


http://gerrit.cloudera.org:8080/#/c/4011/1/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 799
Revert this change. flake8 likes 2 lines between classes anyway.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has uploaded a new patch set (#2).

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................

IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME

Testing:

* Hive integration tested locally by invoking the data generator (./data-generator.py
--db-name=functional --use-hive --min-row-count=50 --max-row-count=100
--storage-file-formats textfile --use-postgresql --postgresql-user stakiar) and the
discrepancy checker (./discrepancy-checker.py --db-name=functional --use-hive
--use-postgresql --postgresql-user stakiar --test-db-type HIVE --timeout 300
--query-count 50 --profile hive)
* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
3 files changed, 57 insertions(+), 21 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has uploaded a new patch set (#3).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
3 files changed, 61 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Sahil Takiar (Code Review)" <ge...@cloudera.org>.
Sahil Takiar has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/11/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 816: 
> I meant this method should be removed.
Ah I see, my mistake you are correct. I removed it and it still works. Thanks.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Sahil Takiar <st...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4011

to look at the new patch set (#10).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 92 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/10
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 5:

Sahil, can you take another look at the tests? I get a failure when I check out your patch.

https://gist.github.com/mikesbrown/94112d5364ce5a6d8ecd864226261fc6

I'm willing to concede the flakiness of the tests btw, but I am able to successfully get the tests to pass on master.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Michael Brown (Code Review)" <ge...@cloudera.org>.
Michael Brown has posted comments on this change.

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4011/2//COMMIT_MSG
Commit Message:

PS2, Line 7: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
I prefer commit messages that, after the bug, speak of the code section changed. So something like:

"IMPALA-3890: qgen: reenable Hive as a target database"


PS2, Line 19: * Hive integration tested locally by invoking the data generator (./data-generator.py
            : --db-name=functional --use-hive --min-row-count=50 --max-row-count=100
            : --storage-file-formats textfile --use-postgresql --postgresql-user stakiar) and the
            : discrepancy checker (./discrepancy-checker.py --db-name=functional --use-hive
            : --use-postgresql --postgresql-user stakiar --test-db-type HIVE --timeout 300
            : --query-count 50 --profile hive)
While it makes the commit message have more lines, it is more readable to write the commands with breaks across options, like this:

  ./data_generator.py \
    --db-name=functional \
    --use-hive \
    --min-row-count=50 \
    --max-row-count=100 \
    --storage-file-formats textfile \
    --use-postgresql \
    --postgresql-user stakiar


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/cli_options.py
File tests/comparison/cli_options.py:

PS2, Line 150:   group.add_argument('--hive-host', default='localhost',
             :       help="The name of the host running the HS2")
             :   group.add_argument("--hive-port", default=10000, type=int,
             :       help="The port of HiveServer2")
Why not derive these defaults from symbols imported from cluster.py? (The constants I've asked you to create in that file)


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS2, Line 166:   def __init__(self, hive_host, hive_port, num_impalads=3):
This changes the MiniCluster constructor interface for all callers. It also implicitly changes the cli_options.create_cluster() call, which has several callers.

What do you think about defining 127.0.0.1 and 11050 (from the old code L179) as default hive host and port as constants at the top of this module? Then make hive_host and hive_port on this line optional, defaulting to those constants.


PS2, Line 178:     node_conf_dir = os.environ["HADOOP_CONF_DIR"]
Thanks for doing the cleanups like this one.


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/data_generator.py
File tests/comparison/data_generator.py:

PS2, Line 101:   def populate_db(self, table_count, postgresql_conn=None, isHive=False):
We don't use variable names of the format "isHive" typically, so can you change this to is_hive?


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has uploaded a new patch set (#7).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
4 files changed, 66 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/7
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4011

to look at the new patch set (#8).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 91 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/8
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/3/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS3, Line 56: DEFAULT_HIVE_PORT = 11050
> This should be reverted back to 11050 to match what's already in use in the
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 4:

> I'd like you to run the small suite of cluster tests, but you can't
 > yet until you rebase on top of https://gerrit.cloudera.org/#/c/4404/
 > I realize there aren't that many, and they may not find anything,
 > but it's a habit we need to start. Thanks.

Will do, looks like your patch just got merged. I will rebase my patch and then run the tests. Will update this review once I'm done.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
Hello Michael Brown,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4011

to look at the new patch set (#11).

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................

IMPALA-3980: qgen: re-enable Hive as a target database

Changes:

* Added hive cli options back in (removed in commit "Stress test: Various changes")
* Modifications so that if --use-hive is specified, a Hive connection is actually created
* A few minor bug fixes so that the RQG can be run locally
* Modified MiniCluster to use HADOOP_CONF_DIR and HIVE_CONF_DIR rather than a hard-coded
file under IMPALA_HOME
* Fixed fe/src/test/resources/hive-default.xml so that it is a valid XML file, it was
missing a few element terminators that cause an exception in the cluster.py file

Testing:

* Hive integration tested locally by invoking the data generator via the command:

./data-generator.py \
--db-name=functional \
--use-hive \
--min-row-count=50 \
--max-row-count=100 \
--storage-file-formats textfile \
--use-postgresql \
--postgresql-user stakiar

and the discrepancy checker via the command:

./discrepancy-checker.py \
--db-name=functional \
--use-hive \
--use-postgresql \
--postgresql-user stakiar \
--test-db-type HIVE \
--timeout 300 \
--query-count 50 \
--profile hive

* The output of the above two commands is essentially the same as the Impala output,
however, about 20% of the queries will fail when the discrepancy checker is run
* Regression testing done by running Leopard in a local VM running Ubuntu 14.04, and by
running the discrepancy checker against Impala while inside an Impala Docker container

Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
---
M fe/src/test/resources/hive-default.xml
M tests/comparison/cli_options.py
M tests/comparison/cluster.py
M tests/comparison/data_generator.py
M tests/comparison/db_connection.py
5 files changed, 126 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/11/4011/11
-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/4011/2//COMMIT_MSG
Commit Message:

PS2, Line 7: IMPALA-3980: qgen: re-enable Hive as a target database
> I prefer commit messages that, after the bug, speak of the code section cha
Done


PS2, Line 19: * Hive integration tested locally by invoking the data generator via the command:
            : 
            : ./data-generator.py \
            : --db-name=functional \
            : --use-hive \
            : --min-row-count=50 \
> While it makes the commit message have more lines, it is more readable to w
Done


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/cli_options.py
File tests/comparison/cli_options.py:

PS2, Line 150:   group.add_argument('--hive-host', default=DEFAULT_HIVE_HOST,
             :       help="The name of the host running the HS2")
             :   group.add_argument("--hive-port", default=DEFAULT_HIVE_PORT, type=int,
             :       help="The port of HiveServer2")
> Why not derive these defaults from symbols imported from cluster.py? (The c
Done


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/cluster.py
File tests/comparison/cluster.py:

PS2, Line 166: class MiniCluster(Cluster):
> This changes the MiniCluster constructor interface for all callers. It also
Done


PS2, Line 178:   def _init_local_hadoop_conf_dir(self):
> Thanks for doing the cleanups like this one.
No problem! It makes the code a lot easier to run locally.


http://gerrit.cloudera.org:8080/#/c/4011/2/tests/comparison/data_generator.py
File tests/comparison/data_generator.py:

PS2, Line 101:   def populate_db(self, table_count, postgresql_conn=None, is_hive=False)
> We don't use variable names of the format "isHive" typically, so can you ch
Done


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

Change subject: IMPALA-3980: Re-enable Hive as a target database for the Random Query Generator
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4011/1//COMMIT_MSG
Commit Message:

PS1, Line 11: * Added hive cli options back in (removed in 9608085d0eb6da233c877830b404ad748f45e20d)
> Refer to commit hash f288867833b6f2953ef570217b953c9d523435b4 here, since t
Even better, don't refer to hashes: they can and will change across branches. Refer to commit titles if you really need to call out a specific commit.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Taras Bobrovytsky (Code Review)" <ge...@cloudera.org>.
Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/4011/7/tests/comparison/cluster.py
File tests/comparison/cluster.py:

Line 185:     other_conf_dir = os.environ["HIVE_CONF_DIR"]
> I run the Hive tests against a local instance of Hive, so setting this to H
ok, what do you think about creating a new HiveMiniCluster object that inherits from MiniCluster (or maybe even directly from the Cluster class?) and making changes to it?


http://gerrit.cloudera.org:8080/#/c/4011/10/tests/comparison/data_generator.py
File tests/comparison/data_generator.py:

Line 156:     elif self.db_engine is db_connection.HIVE:
An exception should be raised if db_engine is not Impala or Hive.


http://gerrit.cloudera.org:8080/#/c/4011/10/tests/comparison/db_connection.py
File tests/comparison/db_connection.py:

Line 792: class HiveCursor(ImpalaCursor):
Is this class being used anywhere?
I think you want to override _CURSOR_CLASS in HiveConnection and specify this class there.


Line 815:   def cursor(self):
What is the point of overriding the cursor method here? It does exactly the same thing the cursor method in the parent class.


-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 8:

@Taras, comments addressed, patch updated and re-tested.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3980: qgen: re-enable Hive as a target database

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
stakiar@cloudera.com has posted comments on this change.

Change subject: IMPALA-3980: qgen: re-enable Hive as a target database
......................................................................


Patch Set 6:

> Uploaded patch set 6.

Hey Michael,

Apologies, yes you are correct, the tests were failing with this patch. It seems my dev setup wasn't fully setup properly. Originally, I was trying to avoid building Impala fully, so I made some modifications to get the Python code to run locally. The tests were passing locally, but I realize this probably isn't a great approach if I want to reliably run these unit tests. I've set up a proper Impala dev environment on a remote machine. Once I did that I was able to re-produce the error. The hive-default.xml file under fe/src/test/resources/ is not a valid XML file. It was missing some element terminators. Once I fixed those the tests starting working. I have updated the patch.

-- 
To view, visit http://gerrit.cloudera.org:8080/4011
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb1199b50a5b65c21de7876fb70cc03bda1a9b46
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: stakiar@cloudera.com
Gerrit-Reviewer: David Knupp <dk...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Michael Brown <mi...@cloudera.com>
Gerrit-Reviewer: Taras Bobrovytsky <tb...@cloudera.com>
Gerrit-Reviewer: stakiar@cloudera.com
Gerrit-HasComments: No