[Impala-ASF-CR] Clean up generation of XML configuration files
Todd Lipcon has submitted this change and it was merged. (
http://gerrit.cloudera.org:8080/12930 )
Change subject: Clean up generation of XML configuration files
..
Clean up generation of XML configuration files
hive-site.xml and sentry-site.xml in particular had grown multiple
slightly-different variants, differing only in a few small pieces. This
was difficult to maintain: in fact, while attempting to clean them up I
found a number of places that the MySQL and Postgres versions of
hive-site had diverged for no apparent reason.
This moves away from using the sed-based templating for these
configuration files, and instead uses python as a poor man's template
system. That enables much simpler conditional logic.
I briefly considered XSLT for this, but decided that Python is probably
easier for the average developer to follow, modify, and debug.
Along the way, I removed a few flags which appear to be no longer used
by Hive 2 or later, and a few items which were already commented out in
the previous template:
- hive.stats.dbclass
- hive.stats.dbconnectionstring
- hive.stats.jdbcdriver
These are no longer relevant after HIVE-12164 ("Remove jdbc stats
collection mechanism") in Hive 2.0.
- hive.metastore.rawstore.impl
This has always defaulted to 'ObjectStore' in Hive, so there was no
reason to set it explicitly.
- test.log.dir
- test.src.dir
These were listed in the config in a commented-out section. These were
commented out ever since 2012 when the file was first introduced.
This also fixes the postgres URL to not include a misplaced ';create'
parameter (which applies to Derby but not postgres).
Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47
Reviewed-on: http://gerrit.cloudera.org:8080/12930
Reviewed-by: Fredy Wijaya
Reviewed-by: Tim Armstrong
Tested-by: Impala Public Jenkins
---
M bin/create-test-configuration.sh
A bin/generate_xml_config.py
M bin/jenkins/critique-gerrit-review.py
A fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/mysql-hive-site.xml.template
D fe/src/test/resources/postgresql-hive-site.xml.cdp.template
D fe/src/test/resources/postgresql-hive-site.xml.template
A fe/src/test/resources/sentry-site.xml.py
D fe/src/test/resources/sentry-site.xml.template
D fe/src/test/resources/sentry-site_no_oo.xml.template
D fe/src/test/resources/sentry-site_oo.xml.template
D fe/src/test/resources/sentry-site_oo_nogrant.xml.template
12 files changed, 321 insertions(+), 998 deletions(-)
Approvals:
Fredy Wijaya: Looks good to me, but someone else must approve
Tim Armstrong: Looks good to me, approved
Impala Public Jenkins: Verified
--
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: merged
Gerrit-Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47
Gerrit-Change-Number: 12930
Gerrit-PatchSet: 4
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 3: Verified+1 -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 05 Apr 2019 21:30:18 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 3: Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/3987/ DRY_RUN=true -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Fri, 05 Apr 2019 16:45:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
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 3: (1 comment) http://gerrit.cloudera.org:8080/#/c/12930/3/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/12930/3/bin/jenkins/critique-gerrit-review.py@72 PS3, Line 72: > flake8: E261 at least two spaces before inline comment I'm tempted to disable this warning. I guess it's part of PEP-8 and therefore standard but I find it very hard to see how it makes anything more readable: https://www.python.org/dev/peps/pep-0008/#inline-comments -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 21:40:25 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
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 3: Code-Review+2 -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 21:38:51 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Fredy Wijaya has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 3: Code-Review+1 -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 20:03:31 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 3: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2650/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 19:56:50 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 3: (7 comments) http://gerrit.cloudera.org:8080/#/c/12930/3/bin/jenkins/critique-gerrit-review.py File bin/jenkins/critique-gerrit-review.py: http://gerrit.cloudera.org:8080/#/c/12930/3/bin/jenkins/critique-gerrit-review.py@72 PS3, Line 72: flake8: E261 at least two spaces before inline comment http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py File fe/src/test/resources/hive-site.xml.py: http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@58 PS3, Line 58: 2 flake8: E501 line too long (96 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@67 PS3, Line 67: e flake8: E501 line too long (94 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@82 PS3, Line 82: i flake8: E501 line too long (119 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@92 PS3, Line 92: f flake8: E501 line too long (108 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@93 PS3, Line 93: . flake8: E501 line too long (117 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/3/fe/src/test/resources/hive-site.xml.py@113 PS3, Line 113: t flake8: E501 line too long (112 > 90 characters) -- 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: 3 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 19:31:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
Todd Lipcon has posted comments on this change. (
http://gerrit.cloudera.org:8080/12930 )
Change subject: Clean up generation of XML configuration files
..
Patch Set 2:
(7 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
Done
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, ju
tested it on an el6 box and it appears to work.
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 g
Done. Added a TODO for myself to convert over the remaining -site.xml files
(hbase, ranger-admin)
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 genera
Done
http://gerrit.cloudera.org:8080/#/c/12930/2/bin/generate_xml_config.py@113
PS2, Line 113: os.unlink(tmp_path)
> Does this delete the tmp file as well?
yea, unlink = delete
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
> (you don't need to fix it btw, just couldn't resist commenting)
yea I actually wasted a few minutes because I accidentally fixed the typo and
then some stuff broke :) It's set with this typo elsewhere.
http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py
File fe/src/test/resources/sentry-site.xml.py:
http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py@49
PS2, Line 49: 'sentry.store.jdbc.url':
'jdbc:postgresql://localhost:5432/${SENTRY_POLICY_DB}/;create=true'
> we should fix this JDBC URL: https://gerrit.cloudera.org/c/12894/3/fe/src/t
Done
--
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
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 04 Apr 2019 19:30:49 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
Hello Vihang Karajgaonkar, Fredy Wijaya, Tim Armstrong, Impala Public Jenkins,
Adam Holley,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12930
to look at the new patch set (#3).
Change subject: Clean up generation of XML configuration files
..
Clean up generation of XML configuration files
hive-site.xml and sentry-site.xml in particular had grown multiple
slightly-different variants, differing only in a few small pieces. This
was difficult to maintain: in fact, while attempting to clean them up I
found a number of places that the MySQL and Postgres versions of
hive-site had diverged for no apparent reason.
This moves away from using the sed-based templating for these
configuration files, and instead uses python as a poor man's template
system. That enables much simpler conditional logic.
I briefly considered XSLT for this, but decided that Python is probably
easier for the average developer to follow, modify, and debug.
Along the way, I removed a few flags which appear to be no longer used
by Hive 2 or later, and a few items which were already commented out in
the previous template:
- hive.stats.dbclass
- hive.stats.dbconnectionstring
- hive.stats.jdbcdriver
These are no longer relevant after HIVE-12164 ("Remove jdbc stats
collection mechanism") in Hive 2.0.
- hive.metastore.rawstore.impl
This has always defaulted to 'ObjectStore' in Hive, so there was no
reason to set it explicitly.
- test.log.dir
- test.src.dir
These were listed in the config in a commented-out section. These were
commented out ever since 2012 when the file was first introduced.
This also fixes the postgres URL to not include a misplaced ';create'
parameter (which applies to Derby but not postgres).
Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47
---
M bin/create-test-configuration.sh
A bin/generate_xml_config.py
M bin/jenkins/critique-gerrit-review.py
A fe/src/test/resources/hive-site.xml.py
D fe/src/test/resources/mysql-hive-site.xml.template
D fe/src/test/resources/postgresql-hive-site.xml.cdp.template
D fe/src/test/resources/postgresql-hive-site.xml.template
A fe/src/test/resources/sentry-site.xml.py
D fe/src/test/resources/sentry-site.xml.template
D fe/src/test/resources/sentry-site_no_oo.xml.template
D fe/src/test/resources/sentry-site_oo.xml.template
D fe/src/test/resources/sentry-site_oo_nogrant.xml.template
12 files changed, 321 insertions(+), 998 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/12930/3
--
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: newpatchset
Gerrit-Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47
Gerrit-Change-Number: 12930
Gerrit-PatchSet: 3
Gerrit-Owner: Todd Lipcon
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] Clean up generation of XML configuration files
Fredy Wijaya has posted comments on this change. (
http://gerrit.cloudera.org:8080/12930 )
Change subject: Clean up generation of XML configuration files
..
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py
File fe/src/test/resources/sentry-site.xml.py:
http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py@49
PS2, Line 49: 'sentry.store.jdbc.url':
'jdbc:postgresql://localhost:5432/${SENTRY_POLICY_DB}/;create=true'
we should fix this JDBC URL:
https://gerrit.cloudera.org/c/12894/3/fe/src/test/resources/sentry-site.xml.template
--
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
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 04 Apr 2019 19:06:59 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
Vihang Karajgaonkar has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 2: Code-Review+1 (1 comment) Just a question related to cleanup on errors. Rest looks good to me. Thanks for the patch. I think its a lot cleaner now. 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@113 PS2, Line 113: os.unlink(tmp_path) Does this delete the tmp file as well? -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 17:49:40 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
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: (1 comment) 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? (you don't need to fix it btw, just couldn't resist commenting) -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 17:44:23 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
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
Gerrit-Reviewer: Adam Holley
Gerrit-Reviewer: Fredy Wijaya
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong
Gerrit-Reviewer: Todd Lipcon
Gerrit-Reviewer: Vihang Karajgaonkar
Gerrit-Comment-Date: Thu, 04 Apr 2019 17:36:06 +
Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 2: Thanks. I think for this case the "long lines" are probably a good idea for readability vs lots of weird breaking of long strings. I spent some time trying to see if I could disable the "long line" warning on flake8 for a single file, but just ended up finding some bikeshed github issue arguments about whether that's a good feature or not... and I gave up. Looks like we don't have a standard of "flake8 clean" so leaving these for now. -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 17:29:28 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
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: If you want, you can prefetch the bot comments with ./bin/jenkins/critique-gerrit-review.py --dryrun -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 17:11:59 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 2: Build Successful https://jenkins.impala.io/job/gerrit-code-review-checks/2640/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests. -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 16:52:43 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 2: (7 comments) 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@58 PS2, Line 58: 2 flake8: E501 line too long (96 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@67 PS2, Line 67: e flake8: E501 line too long (94 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@82 PS2, Line 82: i flake8: E501 line too long (119 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@92 PS2, Line 92: f flake8: E501 line too long (108 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@93 PS2, Line 93: . flake8: E501 line too long (117 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/hive-site.xml.py@113 PS2, Line 113: t flake8: E501 line too long (112 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py File fe/src/test/resources/sentry-site.xml.py: http://gerrit.cloudera.org:8080/#/c/12930/2/fe/src/test/resources/sentry-site.xml.py@49 PS2, Line 49: r flake8: E501 line too long (95 > 90 characters) -- 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 Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 16:19:11 + Gerrit-HasComments: Yes
[Impala-ASF-CR] Clean up generation of XML configuration files
Hello Vihang Karajgaonkar, Fredy Wijaya, Tim Armstrong, Impala Public Jenkins, Adam Holley, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/12930 to look at the new patch set (#2). Change subject: Clean up generation of XML configuration files .. Clean up generation of XML configuration files hive-site.xml and sentry-site.xml in particular had grown multiple slightly-different variants, differing only in a few small pieces. This was difficult to maintain: in fact, while attempting to clean them up I found a number of places that the MySQL and Postgres versions of hive-site had diverged for no apparent reason. This moves away from using the sed-based templating for these configuration files, and instead uses python as a poor man's template system. That enables much simpler conditional logic. I briefly considered XSLT for this, but decided that Python is probably easier for the average developer to follow, modify, and debug. Along the way, I removed a few flags which appear to be no longer used by Hive 2 or later, and a few items which were already commented out in the previous template. Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47 --- M bin/create-test-configuration.sh A bin/generate_xml_config.py A fe/src/test/resources/hive-site.xml.py D fe/src/test/resources/mysql-hive-site.xml.template D fe/src/test/resources/postgresql-hive-site.xml.cdp.template D fe/src/test/resources/postgresql-hive-site.xml.template A fe/src/test/resources/sentry-site.xml.py D fe/src/test/resources/sentry-site.xml.template D fe/src/test/resources/sentry-site_no_oo.xml.template D fe/src/test/resources/sentry-site_oo.xml.template D fe/src/test/resources/sentry-site_oo_nogrant.xml.template 11 files changed, 310 insertions(+), 997 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/12930/2 -- 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: newpatchset Gerrit-Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47 Gerrit-Change-Number: 12930 Gerrit-PatchSet: 2 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 1: Build Failed https://jenkins.impala.io/job/gerrit-code-review-checks/2637/ : Initial code review checks failed. See linked job for details on the failure. -- 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: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Adam Holley Gerrit-Reviewer: Fredy Wijaya Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 06:48:08 + Gerrit-HasComments: No
[Impala-ASF-CR] Clean up generation of XML configuration files
Hello Vihang Karajgaonkar, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/12930 to review the following change. Change subject: Clean up generation of XML configuration files .. Clean up generation of XML configuration files hive-site.xml and sentry-site.xml in particular had grown multiple slightly-different variants, differing only in a few small pieces. This was difficult to maintain: in fact, while attempting to clean them up I found a number of places that the MySQL and Postgres versions of hive-site had diverged for no apparent reason. This moves away from using the sed-based templating for these configuration files, and instead uses python as a poor man's template system. That enables much simpler conditional logic. I briefly considered XSLT for this, but decided that Python is probably easier for the average developer to follow, modify, and debug. Along the way, I removed a few flags which appear to be no longer used by Hive 2 or later, and a few items which were already commented out in the previous template. Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47 --- M bin/create-test-configuration.sh A bin/generate_xml_config.py A fe/src/test/resources/hive-site.xml.py D fe/src/test/resources/mysql-hive-site.xml.template D fe/src/test/resources/postgresql-hive-site.xml.cdp.template D fe/src/test/resources/postgresql-hive-site.xml.template A fe/src/test/resources/sentry-site.xml.py D fe/src/test/resources/sentry-site.xml.template D fe/src/test/resources/sentry-site_no_oo.xml.template D fe/src/test/resources/sentry-site_oo.xml.template D fe/src/test/resources/sentry-site_oo_nogrant.xml.template 11 files changed, 280 insertions(+), 997 deletions(-) git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/30/12930/1 -- 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: newchange Gerrit-Change-Id: Ief4434d80baae0fd7be7ffe7b2e07bae1ac45e47 Gerrit-Change-Number: 12930 Gerrit-PatchSet: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Vihang Karajgaonkar
[Impala-ASF-CR] Clean up generation of XML configuration files
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/12930 ) Change subject: Clean up generation of XML configuration files .. Patch Set 1: (19 comments) http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py File bin/generate_xml_config.py: http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@18 PS1, Line 18: t flake8: E501 line too long (99 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@33 PS1, Line 33: def _substitute_env_vars(s): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@38 PS1, Line 38: def dump_config(d, source_path, out): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@46 PS1, Line 46: flake8: W293 blank line contains whitespace http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@46 PS1, Line 46: line has trailing whitespace http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@57 PS1, Line 57: ) flake8: E501 line too long (91 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@66 PS1, Line 66: def main(): flake8: E302 expected 2 blank lines, found 1 http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@81 PS1, Line 81: e flake8: E722 do not use bare except' http://gerrit.cloudera.org:8080/#/c/12930/1/bin/generate_xml_config.py@86 PS1, Line 86: if __name__ == "__main__": flake8: E305 expected 2 blank lines after class or function definition, found 1 http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py File fe/src/test/resources/hive-site.xml.py: http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@58 PS1, Line 58: 2 flake8: E501 line too long (96 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@66 PS1, Line 66: # flake8: E131 continuation line unaligned for hanging indent http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@67 PS1, Line 67: e flake8: E501 line too long (94 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@82 PS1, Line 82: ' flake8: E131 continuation line unaligned for hanging indent http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@82 PS1, Line 82: i flake8: E501 line too long (119 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@92 PS1, Line 92: f flake8: E501 line too long (108 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@93 PS1, Line 93: . flake8: E501 line too long (117 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@113 PS1, Line 113: t flake8: E501 line too long (112 > 90 characters) http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/hive-site.xml.py@118 PS1, Line 118: flake8: W391 blank line at end of file http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/sentry-site.xml.py File fe/src/test/resources/sentry-site.xml.py: http://gerrit.cloudera.org:8080/#/c/12930/1/fe/src/test/resources/sentry-site.xml.py@49 PS1, Line 49: r flake8: E501 line too long (95 > 90 characters) -- 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: 1 Gerrit-Owner: Todd Lipcon Gerrit-Reviewer: Impala Public Jenkins Gerrit-Reviewer: Tim Armstrong Gerrit-Reviewer: Vihang Karajgaonkar Gerrit-Comment-Date: Thu, 04 Apr 2019 05:51:45 + Gerrit-HasComments: Yes
