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: [email protected] Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-HasComments: Yes
