Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12930 )

Change subject: Clean up generation of XML configuration files
......................................................................


Patch Set 2:

(5 comments)

Ok no worries, yeah that makes sense. You could add the file to the list in 
bin/jenkins/critique-gerrit-review.py, I have no issue with that if it's 
problematic.

Overall this looks good, mainly just some questions about comments.

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

http://gerrit.cloudera.org:8080/#/c/12930/2//COMMIT_MSG@24
PS2, Line 24: the previous template.
Could you list the flags out? Just to make it easier to understand. I did a 
manual comparison and got:

* hive.stats.dbclass
* hive.metastore.rawstore.impl


http://gerrit.cloudera.org:8080/#/c/12930/2//COMMIT_MSG@25
PS2, Line 25:
We should make sure to test on CentOS 6 since that still has python 2.6, just 
in case there's something incompatible in the new Python code. :sadpanda:.


http://gerrit.cloudera.org:8080/#/c/12930/2/bin/create-test-configuration.sh
File bin/create-test-configuration.sh:

http://gerrit.cloudera.org:8080/#/c/12930/2/bin/create-test-configuration.sh@32
PS2, Line 32: function generate_config {
It would be good to leave a comment here or below with thoughts about why 
generate_config would be used over the python-based templating (or if we want 
to port everything to python-based templating eventually).

Otherwise people will be confused about why two mechanisms exist.


http://gerrit.cloudera.org:8080/#/c/12930/2/bin/generate_xml_config.py
File bin/generate_xml_config.py:

http://gerrit.cloudera.org:8080/#/c/12930/2/bin/generate_xml_config.py@80
PS2, Line 80:       """ % dict(source_path=os.path.abspath(source_path))
New-style formatting - .format(source_path=...) and {source_path} is generally 
preferred in new code over %. Not a hard requirement but I think it should be 
easy to change over here and elsewhere.


http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py
File fe/src/test/resources/hive-site.xml.py:

http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@69
PS2, Line 69: HBSE
I'm so confused by this abbreviation - why omit the A?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47
Gerrit-Change-Number: 12930
Gerrit-PatchSet: 2
Gerrit-Owner: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Adam Holley <ahol...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fwij...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Comment-Date: Thu, 04 Apr 2019 17:36:06 +0000
Gerrit-HasComments: Yes

Reply via email to