Henry Robinson has posted comments on this change. Change subject: IMPALA-3306: Add command-line flags to set S3 access configurations ......................................................................
Patch Set 2: Code-Review+1 (9 comments) http://gerrit.cloudera.org:8080/#/c/2850/2/be/src/runtime/hdfs-fs-cache.cc File be/src/runtime/hdfs-fs-cache.cc: Line 45: return Status(Substitute("Could not run command '$0'. " .. " to retrieve S3 key". Line 48: LOG(INFO) << "Command '" << s3a_key_cmd << "' executed successfully."; Prefer: "S3 key command '...' completed successfully" Line 57: RETURN_IF_ERROR(GetS3Key(FLAGS_s3a_access_key_cmd, &s3a_access_key_)); Let's remove GetS3Key() and inline the logic here: that way you can customize the error messages further. http://gerrit.cloudera.org:8080/#/c/2850/2/be/src/runtime/hdfs-fs-cache.h File be/src/runtime/hdfs-fs-cache.h: Line 45: and retrieves the S3A access and secret keys if the : /// corressponding commands are provided I would remove this; it's part of initialising the cache. Line 64: /// S3A access key retrieved by running command in Init(). "If blank, the default value is taken from the local Hadoop client configuration". http://gerrit.cloudera.org:8080/#/c/2850/2/be/src/util/os-util.h File be/src/util/os-util.h: Line 45: /// the first 1k of output otherwise. Update comment. http://gerrit.cloudera.org:8080/#/c/2850/2/tests/custom_cluster/test_s3a_access.py File tests/custom_cluster/test_s3a_access.py: Line 23: BAD_KEY_FILE = "/tmp/s3a_access_tmp_cmd" Probably better to use tempfile in case this ever gets run concurrently on the same machine (see https://docs.python.org/2/library/tempfile.html). Line 43: nit: remove trailing whitespace Line 63: which should have the correct keys.''' Add a TODO mentioning that we don't have the infrastructure to test the case where the keys are correct yet -- 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: 2 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
