impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philip Zeyliger (Code Review)" <>
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. (

Change subject: IMPALA-6067: Enable S3 access via IAM roles for EC2 VMs

Patch Set 4: Code-Review+1


Works for me. Thanks! The two nits below are very minor.
File bin/
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.
PS4, Line 353:     DEFAULT_FS="s3a://${S3_BUCKET}"
             :     export DEFAULT_FS

export DEFAULT_FS=...

would work. You do that in lines 330 and 340, so it's consistent.

To view, visit
To unsubscribe, visit

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

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