impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-6067: Enable S3 access via IAM roles for EC2 VMs
Date Fri, 01 Dec 2017 22:55:54 GMT
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.gaal@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.behm@cloudera.com>
Gerrit-Reviewer: David Knupp <dknupp@cloudera.com>
Gerrit-Reviewer: Jim Apple <jbapple-impala@apache.org>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Lars Volker <lv@cloudera.com>
Gerrit-Reviewer: Laszlo Gaal <laszlo.gaal@cloudera.com>
Gerrit-Reviewer: Michael Brown <mikeb@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Fri, 01 Dec 2017 22:55:54 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message