You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hive.apache.org by Peter Vary <pv...@cloudera.com> on 2016/08/25 18:01:48 UTC
Review Request 51435: HIVE-14426 Extensive logging on info level in
WebHCat
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/
-----------------------------------------------------------
Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
Bugs: HIVE-14426
https://issues.apache.org/jira/browse/HIVE-14426
Repository: hive-git
Description
-------
Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
Used Utilities.maskIfPassword to remove sensitive information from property dump
Diffs
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
Diff: https://reviews.apache.org/r/51435/diff/
Testing
-------
Manually - checked both
Thanks,
Peter Vary
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147447
-----------------------------------------------------------
Ship it!
LGTM
- Chaoyu Tang
On Aug. 31, 2016, 3:32 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 3:32 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/LogUtils.java 599e798
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> common/src/test/org/apache/hadoop/hive/common/TestLogUtils.java PRE-CREATION
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java a542dc4
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java ac922ce
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 8e5c0da
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 0786f9b
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/
-----------------------------------------------------------
(Updated Aug. 31, 2016, 3:32 p.m.)
Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
Bugs: HIVE-14426
https://issues.apache.org/jira/browse/HIVE-14426
Repository: hive-git
Description
-------
Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
Used Utilities.maskIfPassword to remove sensitive information from property dump
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/LogUtils.java 599e798
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
common/src/test/org/apache/hadoop/hive/common/TestLogUtils.java PRE-CREATION
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java a542dc4
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java ac922ce
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 8e5c0da
ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 0786f9b
Diff: https://reviews.apache.org/r/51435/diff/
Testing
-------
Manually - checked both
Thanks,
Peter Vary
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/
-----------------------------------------------------------
(Updated Aug. 31, 2016, 3:06 p.m.)
Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
Bugs: HIVE-14426
https://issues.apache.org/jira/browse/HIVE-14426
Repository: hive-git
Description
-------
Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
Used Utilities.maskIfPassword to remove sensitive information from property dump
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/common/LogUtils.java 599e798
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
common/src/test/org/apache/hadoop/hive/common/TestLogUtils.java PRE-CREATION
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java a542dc4
ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java ac922ce
ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 8e5c0da
ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 0786f9b
Diff: https://reviews.apache.org/r/51435/diff/
Testing
-------
Manually - checked both
Thanks,
Peter Vary
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 31, 2016, 1:48 a.m., Chaoyu Tang wrote:
> > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java, line 488
> > <https://reviews.apache.org/r/51435/diff/3/?file=1488428#file1488428line488>
> >
> > I missed this in last review.
> > Utilities class is form package ql and packaged in hive-exec.jar, I am not sure currently if this jar is mandatory to WebHCat server or not in runtime. If not, will that mean we need an additional runtime jar (hive-exec.jar)?
> > Is it better to refactor the maskIfPassword from ql to common package?
>
> Peter Vary wrote:
> I did not have to add any new dependency to the pom.xml - the Utilities was just there :)
>
> So I checked it now, and it is coming from hive-hcatalog-core, which is directly depending on it.
> - If nothing else, you are right, that we should change the pom.xml of svr, to reflect the direct dependency now.
> - Or we could refactor it from the ql package as you suggested.
>
> What is your suggestion? Which direction should we move?
>
> Thanks,
> Peter
>
> Chaoyu Tang wrote:
> For me I might move it to common package.
Refactored, thanks for the suggestion
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147400
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 31, 2016, 1:48 a.m., Chaoyu Tang wrote:
> > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java, line 488
> > <https://reviews.apache.org/r/51435/diff/3/?file=1488428#file1488428line488>
> >
> > I missed this in last review.
> > Utilities class is form package ql and packaged in hive-exec.jar, I am not sure currently if this jar is mandatory to WebHCat server or not in runtime. If not, will that mean we need an additional runtime jar (hive-exec.jar)?
> > Is it better to refactor the maskIfPassword from ql to common package?
I did not have to add any new dependency to the pom.xml - the Utilities was just there :)
So I checked it now, and it is coming from hive-hcatalog-core, which is directly depending on it.
- If nothing else, you are right, that we should change the pom.xml of svr, to reflect the direct dependency now.
- Or we could refactor it from the ql package as you suggested.
What is your suggestion? Which direction should we move?
Thanks,
Peter
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147400
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
> On Aug. 31, 2016, 1:48 a.m., Chaoyu Tang wrote:
> > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java, line 488
> > <https://reviews.apache.org/r/51435/diff/3/?file=1488428#file1488428line488>
> >
> > I missed this in last review.
> > Utilities class is form package ql and packaged in hive-exec.jar, I am not sure currently if this jar is mandatory to WebHCat server or not in runtime. If not, will that mean we need an additional runtime jar (hive-exec.jar)?
> > Is it better to refactor the maskIfPassword from ql to common package?
>
> Peter Vary wrote:
> I did not have to add any new dependency to the pom.xml - the Utilities was just there :)
>
> So I checked it now, and it is coming from hive-hcatalog-core, which is directly depending on it.
> - If nothing else, you are right, that we should change the pom.xml of svr, to reflect the direct dependency now.
> - Or we could refactor it from the ql package as you suggested.
>
> What is your suggestion? Which direction should we move?
>
> Thanks,
> Peter
For me I might move it to common package.
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147400
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147400
-----------------------------------------------------------
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java (line 488)
<https://reviews.apache.org/r/51435/#comment214543>
I missed this in last review.
Utilities class is form package ql and packaged in hive-exec.jar, I am not sure currently if this jar is mandatory to WebHCat server or not in runtime. If not, will that mean we need an additional runtime jar (hive-exec.jar)?
Is it better to refactor the maskIfPassword from ql to common package?
- Chaoyu Tang
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/
-----------------------------------------------------------
(Updated Aug. 30, 2016, 6:34 a.m.)
Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
Bugs: HIVE-14426
https://issues.apache.org/jira/browse/HIVE-14426
Repository: hive-git
Description
-------
Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
Used Utilities.maskIfPassword to remove sensitive information from property dump
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
Diff: https://reviews.apache.org/r/51435/diff/
Testing
-------
Manually - checked both
Thanks,
Peter Vary
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
>
> Peter Vary wrote:
> Thanks Chaoyu!
>
> You are absolutely right!
> I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
>
> Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
> HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
>
> Thanks for the catch.
>
> Any other idea? I might have missed something more - hcatalog is new for me.
>
> Thanks,
> Peter
>
> Chaoyu Tang wrote:
> Hi Peter,
> Here is what I understand the code:
> AppConf loads the configurations from xml files specified in HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include hive-site.xml, so it might contain the proprety such as S3 credentials which are probably specified in hdfs-site.xml. Since appConf does not defined the property hive.conf.hidden.list, so
> the line String hiddenListStr = HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST);
> returns null when HiveConfUtil.dumpConfig(this, sb) is called in AppConf.dumpEnvironent and the passed in parameter configuration is AppConf instance in this case. Therefore the S3 credentials in this AppConf could not be stripped.
> The new change only adds (or appends) the hiveConf hive.conf.hidden.list key/value pair to the value of property HIVE_PROPS_NAME (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined in AppConf and the property like S3 credentials are still not stripped even they have been specified in hiveConf hidden list. Does it make sense? So in order to strip the properties speified in hive.conf.hidden.list, the property hive.conf.hidden.list should be added to AppConf, right?
>
> Peter Vary wrote:
> Hi Chaoyu,
>
> When starting the Main.java from Intellij in debug mode, I found the following:
> - AppConf loads, as you wrote the HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES - no hive-site.xml here
> - If the hive.conf.hidden.list is in any of the files above, then it is used (not in the default case - probably not a good idea to have this value there anyway)
> - If my understanding is correct, if HCatalog uses Hive Metastore, then it expects to have hive-site.xml on classpath - see the comments of the AppConfig.handleHiveProperties method - if this is true, then we are ok, and the hive.conf.hidden.list is set now, and the new patch (with changes after your previous comment) will copy the value to the configuration, and the credentials are removed
> - If hive-site.xml is not on the classpath, then I am not sure what will happen when we try to create a HiveConf object in the first line of the handleHiveProperties method, but if no exception is thrown, then the HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST) will return the default value defined in HiveConf object - at least this is what I saw during my tests.
>
> Most probably you know the HCatalog better than me, do you think it is a valid usecase not to have a hive-conf.xml on the classpath? If so, what should be our solution here? Adding a hive.conf. variables to the webhcat-site.xml files? Add a new templeton specific configuration variable?
>
> Thanks for your time, and review,
> Peter
"HiveConf hiveConf = new HiveConf()" in handleHiveProperties loads hive-site.xml (if any) to its method variable hiveConf, but not the "this" AppConf, right? so the "this" (AppConf) passed to HiveConfUtil.dumpConfig(this, sb) does still not have the HIVE_CONF_HIDDEN_LIST defined unless you call something like
set(HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST.varname, HiveConf.getVar(hiveConf, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST)) to copy the property from hiveConf to this AppConf. handleHiveProperties only appends the HIVE_CONF_HIDDEN_LIST (key=value) from hiveConf to the value of AppConf property HIVE_PROPS_NAME, right?
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
>
> Peter Vary wrote:
> Thanks Chaoyu!
>
> You are absolutely right!
> I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
>
> Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
> HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
>
> Thanks for the catch.
>
> Any other idea? I might have missed something more - hcatalog is new for me.
>
> Thanks,
> Peter
>
> Chaoyu Tang wrote:
> Hi Peter,
> Here is what I understand the code:
> AppConf loads the configurations from xml files specified in HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include hive-site.xml, so it might contain the proprety such as S3 credentials which are probably specified in hdfs-site.xml. Since appConf does not defined the property hive.conf.hidden.list, so
> the line String hiddenListStr = HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST);
> returns null when HiveConfUtil.dumpConfig(this, sb) is called in AppConf.dumpEnvironent and the passed in parameter configuration is AppConf instance in this case. Therefore the S3 credentials in this AppConf could not be stripped.
> The new change only adds (or appends) the hiveConf hive.conf.hidden.list key/value pair to the value of property HIVE_PROPS_NAME (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined in AppConf and the property like S3 credentials are still not stripped even they have been specified in hiveConf hidden list. Does it make sense? So in order to strip the properties speified in hive.conf.hidden.list, the property hive.conf.hidden.list should be added to AppConf, right?
Hi Chaoyu,
When starting the Main.java from Intellij in debug mode, I found the following:
- AppConf loads, as you wrote the HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES - no hive-site.xml here
- If the hive.conf.hidden.list is in any of the files above, then it is used (not in the default case - probably not a good idea to have this value there anyway)
- If my understanding is correct, if HCatalog uses Hive Metastore, then it expects to have hive-site.xml on classpath - see the comments of the AppConfig.handleHiveProperties method - if this is true, then we are ok, and the hive.conf.hidden.list is set now, and the new patch (with changes after your previous comment) will copy the value to the configuration, and the credentials are removed
- If hive-site.xml is not on the classpath, then I am not sure what will happen when we try to create a HiveConf object in the first line of the handleHiveProperties method, but if no exception is thrown, then the HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST) will return the default value defined in HiveConf object - at least this is what I saw during my tests.
Most probably you know the HCatalog better than me, do you think it is a valid usecase not to have a hive-conf.xml on the classpath? If so, what should be our solution here? Adding a hive.conf. variables to the webhcat-site.xml files? Add a new templeton specific configuration variable?
Thanks for your time, and review,
Peter
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
>
> Peter Vary wrote:
> Thanks Chaoyu!
>
> You are absolutely right!
> I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
>
> Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
> HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
>
> Thanks for the catch.
>
> Any other idea? I might have missed something more - hcatalog is new for me.
>
> Thanks,
> Peter
>
> Chaoyu Tang wrote:
> Hi Peter,
> Here is what I understand the code:
> AppConf loads the configurations from xml files specified in HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include hive-site.xml, so it might contain the proprety such as S3 credentials which are probably specified in hdfs-site.xml. Since appConf does not defined the property hive.conf.hidden.list, so
> the line String hiddenListStr = HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST);
> returns null when HiveConfUtil.dumpConfig(this, sb) is called in AppConf.dumpEnvironent and the passed in parameter configuration is AppConf instance in this case. Therefore the S3 credentials in this AppConf could not be stripped.
> The new change only adds (or appends) the hiveConf hive.conf.hidden.list key/value pair to the value of property HIVE_PROPS_NAME (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined in AppConf and the property like S3 credentials are still not stripped even they have been specified in hiveConf hidden list. Does it make sense? So in order to strip the properties speified in hive.conf.hidden.list, the property hive.conf.hidden.list should be added to AppConf, right?
>
> Peter Vary wrote:
> Hi Chaoyu,
>
> When starting the Main.java from Intellij in debug mode, I found the following:
> - AppConf loads, as you wrote the HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES - no hive-site.xml here
> - If the hive.conf.hidden.list is in any of the files above, then it is used (not in the default case - probably not a good idea to have this value there anyway)
> - If my understanding is correct, if HCatalog uses Hive Metastore, then it expects to have hive-site.xml on classpath - see the comments of the AppConfig.handleHiveProperties method - if this is true, then we are ok, and the hive.conf.hidden.list is set now, and the new patch (with changes after your previous comment) will copy the value to the configuration, and the credentials are removed
> - If hive-site.xml is not on the classpath, then I am not sure what will happen when we try to create a HiveConf object in the first line of the handleHiveProperties method, but if no exception is thrown, then the HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST) will return the default value defined in HiveConf object - at least this is what I saw during my tests.
>
> Most probably you know the HCatalog better than me, do you think it is a valid usecase not to have a hive-conf.xml on the classpath? If so, what should be our solution here? Adding a hive.conf. variables to the webhcat-site.xml files? Add a new templeton specific configuration variable?
>
> Thanks for your time, and review,
> Peter
>
> Chaoyu Tang wrote:
> "HiveConf hiveConf = new HiveConf()" in handleHiveProperties loads hive-site.xml (if any) to its method variable hiveConf, but not the "this" AppConf, right? so the "this" (AppConf) passed to HiveConfUtil.dumpConfig(this, sb) does still not have the HIVE_CONF_HIDDEN_LIST defined unless you call something like
> set(HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST.varname, HiveConf.getVar(hiveConf, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST)) to copy the property from hiveConf to this AppConf. handleHiveProperties only appends the HIVE_CONF_HIDDEN_LIST (key=value) from hiveConf to the value of AppConf property HIVE_PROPS_NAME, right?
Thanks Chaoyu,
Finally I got it. I have thought I understand the method comment, but I did not :( It was setting a new config variable, and not copying the original ones.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
>
> Peter Vary wrote:
> Thanks Chaoyu!
>
> You are absolutely right!
> I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
>
> Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
> HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
>
> Thanks for the catch.
>
> Any other idea? I might have missed something more - hcatalog is new for me.
>
> Thanks,
> Peter
Hi Peter,
Here is what I understand the code:
AppConf loads the configurations from xml files specified in HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include hive-site.xml, so it might contain the proprety such as S3 credentials which are probably specified in hdfs-site.xml. Since appConf does not defined the property hive.conf.hidden.list, so
the line String hiddenListStr = HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST);
returns null when HiveConfUtil.dumpConfig(this, sb) is called in AppConf.dumpEnvironent and the passed in parameter configuration is AppConf instance in this case. Therefore the S3 credentials in this AppConf could not be stripped.
The new change only adds (or appends) the hiveConf hive.conf.hidden.list key/value pair to the value of property HIVE_PROPS_NAME (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined in AppConf and the property like S3 credentials are still not stripped even they have been specified in hiveConf hidden list. Does it make sense? So in order to strip the properties speified in hive.conf.hidden.list, the property hive.conf.hidden.list should be added to AppConf, right?
- Chaoyu
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 30, 2016, 6:34 a.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 30, 2016, 6:34 a.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
>
> Peter Vary wrote:
> Thanks Chaoyu!
>
> You are absolutely right!
> I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
>
> Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
> HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
>
> Thanks for the catch.
>
> Any other idea? I might have missed something more - hcatalog is new for me.
>
> Thanks,
> Peter
>
> Chaoyu Tang wrote:
> Hi Peter,
> Here is what I understand the code:
> AppConf loads the configurations from xml files specified in HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES which does not include hive-site.xml, so it might contain the proprety such as S3 credentials which are probably specified in hdfs-site.xml. Since appConf does not defined the property hive.conf.hidden.list, so
> the line String hiddenListStr = HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST);
> returns null when HiveConfUtil.dumpConfig(this, sb) is called in AppConf.dumpEnvironent and the passed in parameter configuration is AppConf instance in this case. Therefore the S3 credentials in this AppConf could not be stripped.
> The new change only adds (or appends) the hiveConf hive.conf.hidden.list key/value pair to the value of property HIVE_PROPS_NAME (templeton.hive.properties), but the HIVE_CONF_HIDDEN_LIST is still undefined in AppConf and the property like S3 credentials are still not stripped even they have been specified in hiveConf hidden list. Does it make sense? So in order to strip the properties speified in hive.conf.hidden.list, the property hive.conf.hidden.list should be added to AppConf, right?
>
> Peter Vary wrote:
> Hi Chaoyu,
>
> When starting the Main.java from Intellij in debug mode, I found the following:
> - AppConf loads, as you wrote the HADOOP_CONF_FILENAMES and TEMPLETON_CONF_FILENAMES - no hive-site.xml here
> - If the hive.conf.hidden.list is in any of the files above, then it is used (not in the default case - probably not a good idea to have this value there anyway)
> - If my understanding is correct, if HCatalog uses Hive Metastore, then it expects to have hive-site.xml on classpath - see the comments of the AppConfig.handleHiveProperties method - if this is true, then we are ok, and the hive.conf.hidden.list is set now, and the new patch (with changes after your previous comment) will copy the value to the configuration, and the credentials are removed
> - If hive-site.xml is not on the classpath, then I am not sure what will happen when we try to create a HiveConf object in the first line of the handleHiveProperties method, but if no exception is thrown, then the HiveConf.getVar(configuration, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST) will return the default value defined in HiveConf object - at least this is what I saw during my tests.
>
> Most probably you know the HCatalog better than me, do you think it is a valid usecase not to have a hive-conf.xml on the classpath? If so, what should be our solution here? Adding a hive.conf. variables to the webhcat-site.xml files? Add a new templeton specific configuration variable?
>
> Thanks for your time, and review,
> Peter
>
> Chaoyu Tang wrote:
> "HiveConf hiveConf = new HiveConf()" in handleHiveProperties loads hive-site.xml (if any) to its method variable hiveConf, but not the "this" AppConf, right? so the "this" (AppConf) passed to HiveConfUtil.dumpConfig(this, sb) does still not have the HIVE_CONF_HIDDEN_LIST defined unless you call something like
> set(HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST.varname, HiveConf.getVar(hiveConf, HiveConf.ConfVars.HIVE_CONF_HIDDEN_LIST)) to copy the property from hiveConf to this AppConf. handleHiveProperties only appends the HIVE_CONF_HIDDEN_LIST (key=value) from hiveConf to the value of AppConf property HIVE_PROPS_NAME, right?
>
> Peter Vary wrote:
> Thanks Chaoyu,
>
> Finally I got it. I have thought I understand the method comment, but I did not :( It was setting a new config variable, and not copying the original ones.
One more thing:
It might be a good idea to add it to the interestingPropNames too, so it could be passed to the client, so the logging could be redacted too.
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 31, 2016, 3:06 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 31, 2016, 3:06 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/common/LogUtils.java 599e798
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> common/src/test/org/apache/hadoop/hive/common/TestLogUtils.java PRE-CREATION
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/AppConfig.java dd1208b
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
> ql/src/java/org/apache/hadoop/hive/ql/exec/Utilities.java a542dc4
> ql/src/java/org/apache/hadoop/hive/ql/exec/mr/MapredLocalTask.java ac922ce
> ql/src/java/org/apache/hadoop/hive/ql/exec/spark/HiveSparkClientFactory.java 8e5c0da
> ql/src/test/org/apache/hadoop/hive/ql/exec/TestUtilities.java 0786f9b
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 27, 2016, 3:20 a.m., Chaoyu Tang wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 72
> > <https://reviews.apache.org/r/51435/diff/2/?file=1486030#file1486030line72>
> >
> > hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
> > The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
Thanks Chaoyu!
You are absolutely right!
I missed this, because when it was not defined in the actual configuration xml-s, then it used the default value - so removed the S3 credentials anyway.
Added it to the interestingPropNames, so it will be copied from the HiveConf, if there is any. If it is not in the following files, or the hiveConf, then we could still use the default value.
HCatalog specific config files: "core-default.xml", "core-site.xml", "mapred-default.xml", "mapred-site.xml", "hdfs-site.xml", "webhcat-default.xml", "webhcat-site.xml"
Thanks for the catch.
Any other idea? I might have missed something more - hcatalog is new for me.
Thanks,
Peter
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
On Aug. 25, 2016, 8:36 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2016, 8:36 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Chaoyu Tang <ct...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review147068
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (line 72)
<https://reviews.apache.org/r/51435/#comment214066>
hive.conf.hidden.list is usually not default specified in configurations (e.g. that from hdfs-site.xml) other than HiveConf, so the hiddenListStr should be retruned as null for these configuraitons, and the properties listed in HiveConf hidden list are still not able to be stripped from this Config, right?
The S3 credentials I think could be defined in the hdfs configuration but they are specified in HiveConf hiddenList, they should be stripped and have we tested them?
- Chaoyu Tang
On Aug. 25, 2016, 8:36 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2016, 8:36 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Gabor Szadovszky <ga...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review146879
-----------------------------------------------------------
LGTM, thanks for the patch.
- Gabor Szadovszky
On Aug. 25, 2016, 8:36 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2016, 8:36 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/
-----------------------------------------------------------
(Updated Aug. 25, 2016, 8:36 p.m.)
Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
Bugs: HIVE-14426
https://issues.apache.org/jira/browse/HIVE-14426
Repository: hive-git
Description
-------
Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
Used Utilities.maskIfPassword to remove sensitive information from property dump
Diffs (updated)
-----
common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
Diff: https://reviews.apache.org/r/51435/diff/
Testing
-------
Manually - checked both
Thanks,
Peter Vary
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Peter Vary <pv...@cloudera.com>.
> On Aug. 25, 2016, 6:42 p.m., Barna Zsombor Klara wrote:
> > common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java, line 73
> > <https://reviews.apache.org/r/51435/diff/1/?file=1485964#file1485964line73>
> >
> > Isn't hiddenSet.clear() redundant here? We just created it on line 71.
Here it is redundant, but had to put is before calling the method.
Thanks for pointing out
> On Aug. 25, 2016, 6:42 p.m., Barna Zsombor Klara wrote:
> > hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java, line 488
> > <https://reviews.apache.org/r/51435/diff/1/?file=1485965#file1485965line488>
> >
> > nit: Use flow API for the last append call as well.
The max linelength forces it to the next line. I think it is nicer this way
- Peter
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review146830
-----------------------------------------------------------
On Aug. 25, 2016, 8:36 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2016, 8:36 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>
Re: Review Request 51435: HIVE-14426 Extensive logging on info level
in WebHCat
Posted by Barna Zsombor Klara <zs...@cloudera.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51435/#review146830
-----------------------------------------------------------
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (line 73)
<https://reviews.apache.org/r/51435/#comment213555>
Isn't hiddenSet.clear() redundant here? We just created it on line 71.
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (line 96)
<https://reviews.apache.org/r/51435/#comment213551>
nit: Values omitted for security reason if present
common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java (line 97)
<https://reviews.apache.org/r/51435/#comment213552>
Instead of String concatenation use .append (maybe even make use of the flow API)
hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java (line 488)
<https://reviews.apache.org/r/51435/#comment213553>
nit: Use flow API for the last append call as well.
LGTM.
Thanks for the patch.
- Barna Zsombor Klara
On Aug. 25, 2016, 6:01 p.m., Peter Vary wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51435/
> -----------------------------------------------------------
>
> (Updated Aug. 25, 2016, 6:01 p.m.)
>
>
> Review request for hive, Chaoyu Tang, Gabor Szadovszky, Sergio Pena, and Barna Zsombor Klara.
>
>
> Bugs: HIVE-14426
> https://issues.apache.org/jira/browse/HIVE-14426
>
>
> Repository: hive-git
>
>
> Description
> -------
>
> Used HiveConf.stripHiddenConfigurations to remove sensitive information from configuration dump. Had to refactor, so it could be applied to simple Configuration objects too, like AppConfig.
>
> Used Utilities.maskIfPassword to remove sensitive information from property dump
>
>
> Diffs
> -----
>
> common/src/java/org/apache/hadoop/hive/conf/HiveConf.java 14a538b
> common/src/java/org/apache/hadoop/hive/conf/HiveConfUtil.java 073a978
> hcatalog/webhcat/svr/src/main/java/org/apache/hive/hcatalog/templeton/tool/TempletonUtils.java 201e647
>
> Diff: https://reviews.apache.org/r/51435/diff/
>
>
> Testing
> -------
>
> Manually - checked both
>
>
> Thanks,
>
> Peter Vary
>
>