Sailesh Mukil has posted comments on this change. Change subject: IMPALA-3306: Add command-line flags to set S3 access configurations ......................................................................
Patch Set 1: (8 comments) 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 Done 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. Done 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 Done 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:: Done Line 45: trim_right(*key); > this pattern is repeated in three places across Impala. How about adding a Done Line 50: void HdfsFsCache::Init( > As discussed, it's ok if the keys are wrong (unless there's a really easy w Done Line 58: s3a_access_key_.clear(); : s3a_secret_key_.clear(); > It's more convenient to have the postcondition of GetS3Key be that the para Done. I've also changed GetS3Key() to return a Status. 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 fla Done -- 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
