Henry Robinson has posted comments on this change. Change subject: IMPALA-3306: Add command-line flags to set S3 access configurations ......................................................................
Patch Set 1: (7 comments) As mentioned, please consider adding a simple smoke-test for this using the custom_cluster tests. http://gerrit.cloudera.org:8080/#/c/2850/1//COMMIT_MSG Commit Message: Line 15: This patch eliminates the need to pass the S3 access and secret keys : through the world readable core-site.xml for Impala. Users can still do that if they like; the idea is not that this makes that mechanism redundant but that they have this convenient option. It's also kinda useful for us. Line 18: P.S: The Hive Metastore still needs the S3 keys in it's core-site as : we do not have control over that. I'd remove this. http://gerrit.cloudera.org:8080/#/c/2850/1/be/src/common/init.cc File be/src/common/init.cc: Line 70: can Remove the last sentence, here and below. It's self-evident that if you're not using S3, you don't need to set it. http://gerrit.cloudera.org:8080/#/c/2850/1/be/src/runtime/hdfs-fs-cache.cc File be/src/runtime/hdfs-fs-cache.cc: Line 36: std:: nit: remove std:: Line 45: trim_right(*key); this pattern is repeated in three places across Impala. How about adding a parameter to RunShellProcess() (or a wrapper method) that does the trimming? Line 58: s3a_access_key_.clear(); : s3a_secret_key_.clear(); It's more convenient to have the postcondition of GetS3Key be that the parameter is unchanged if the method returns false. http://gerrit.cloudera.org:8080/#/c/2850/1/be/src/runtime/hdfs-fs-cache.h File be/src/runtime/hdfs-fs-cache.h: Line 46: static void Init(const string& s3a_access_key_cmd, const string& s3a_secret_key_cmd); rather than make these parameters, why not just have Init() look at the flags themselves? -- To view, visit http://gerrit.cloudera.org:8080/2850 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I2ba103bcb399861683066fd00219d30c180db043 Gerrit-PatchSet: 1 Gerrit-Project: Impala Gerrit-Branch: cdh5-trunk Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Henry Robinson <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
