Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8294 )
Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs ...................................................................... Patch Set 2: (3 comments) http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@294 PS2, Line 294: if (set +x; [[ -z ${AWS_ACCESS_KEY_ID-} && -z ${AWS_SECRET_ACCESS_KEY-} ]]); then > I'm not sure that putting the checks into a separate script would change an Lines 252-264 seem ok - I don't have a problem with setting environment variables in this script. http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@294 PS2, Line 294: if (set +x; [[ -z ${AWS_ACCESS_KEY_ID-} && -z ${AWS_SECRET_ACCESS_KEY-} ]]); then > In most of the use cases the network access is avoided, it happens only if Yeah, I'd question whether "aws ls" belongs in this script either. The performance is one thing but I just generally think we should restrict this script to doing the minimum possible to set up environment variables. I don't see why it should be extended to do heavyweight things to validate the configuration - we don't do anything to validate the vast majority of other variables that are set in this script. It does appear that people have added validations in an ad-hoc way before so if a majority of people think that that's a good idea, I can yield to that. We should also keep in mind that this script is a crappy programming environment, because it's running in the context of the user's shell and we can't use things like "set -x" and have to be careful setting variables that we don't intend to leak into the user's shell. So I think regardless this logic would be more maintainable in a separate script to validate the AWS config. My preference is that we also run that script from elsewhere to keep impala-config.sh lightweight but if other people feel strongly that impala-config.sh should be doing more validation of configs, etc then that's not the worst thing in the world. http://gerrit.cloudera.org:8080/#/c/8294/2/bin/impala-config.sh@303 PS2, Line 303: CURL_ARGS This variable will leak out into the user's shell. -- To view, visit http://gerrit.cloudera.org:8080/8294 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae Gerrit-Change-Number: 8294 Gerrit-PatchSet: 2 Gerrit-Owner: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Jim Apple <jbapple-imp...@apache.org> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Lars Volker <l...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Michael Brown <mi...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 09 Nov 2017 02:21:00 +0000 Gerrit-HasComments: Yes
