Philip Zeyliger 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 4: Code-Review+1 (2 comments) Works for me. Thanks! The two nits below are very minor. http://gerrit.cloudera.org:8080/#/c/8294/4/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/8294/4/bin/impala-config.sh@345 PS4, Line 345: if (set +x; [[ -z ${AWS_SESSION_TOKEN-} ]]); then It's confusing that this is backwards from line 334 and 324. i.e., in the first two cases, the first stanza was "if set, export", but here it's "if unset ... else export". I'd prefer you invert the if, but am happy to not review that again. http://gerrit.cloudera.org:8080/#/c/8294/4/bin/impala-config.sh@353 PS4, Line 353: DEFAULT_FS="s3a://${S3_BUCKET}" : export DEFAULT_FS nit: export DEFAULT_FS=... would work. You do that in lines 330 and 340, so it's consistent. -- 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: 4 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: Fri, 01 Dec 2017 22:55:54 +0000 Gerrit-HasComments: Yes
