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 <[email protected]> Gerrit-Reviewer: Alex Behm <[email protected]> Gerrit-Reviewer: David Knupp <[email protected]> Gerrit-Reviewer: Jim Apple <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Laszlo Gaal <[email protected]> Gerrit-Reviewer: Michael Brown <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Comment-Date: Fri, 01 Dec 2017 22:55:54 +0000 Gerrit-HasComments: Yes
