Sahil Takiar has posted comments on this change. ( http://gerrit.cloudera.org:8080/13020 )
Change subject: IMPALA-8344: Add support for running the minicluster with S3Guard ...................................................................... Patch Set 4: (6 comments) mostly minor comments, overall LGTM pending a few comments http://gerrit.cloudera.org:8080/#/c/13020/4/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/13020/4/bin/impala-config.sh@341 PS4, Line 341: export S3GUARD_METADATASTORE_IMPL="org.apache.hadoop.fs.s3a.s3guard.NullMetadataStore" Is this necessary? I think the default impl uses the NullMetadataStore already, right? http://gerrit.cloudera.org:8080/#/c/13020/4/testdata/bin/load-test-warehouse-snapshot.sh File testdata/bin/load-test-warehouse-snapshot.sh: http://gerrit.cloudera.org:8080/#/c/13020/4/testdata/bin/load-test-warehouse-snapshot.sh@67 PS4, Line 67: hadoop s3guard prune -seconds 1 -meta "dynamodb://${S3GUARD_DYNAMODB_TABLE}" \ is it necessary to prune a newly initialized S3Guard table? http://gerrit.cloudera.org:8080/#/c/13020/4/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/13020/4/tests/common/impala_test_suite.py@a175 PS4, Line 175: Should we delete the S3Client as well? It doesn't look like it is used anywhere else, right? We can remove the boto3 dependency as well, which decreases the number of Python dependencies we have. http://gerrit.cloudera.org:8080/#/c/13020/4/tests/util/hdfs_util.py File tests/util/hdfs_util.py: http://gerrit.cloudera.org:8080/#/c/13020/4/tests/util/hdfs_util.py@158 PS4, Line 158: def __init__(self, filesystem_type="HDFS"): the filesystem_type is just purely for debug messages right? might be useful to mention that http://gerrit.cloudera.org:8080/#/c/13020/4/tests/util/hdfs_util.py@163 PS4, Line 163: hadoop_command = ['hadoop', 'fs'] + command nit: hdfs instead of hadoop http://gerrit.cloudera.org:8080/#/c/13020/4/tests/util/hdfs_util.py@170 PS4, Line 170: def create_file(self, path, file_data, overwrite=True): nit: would be nice to have some docs for these methods, e.g. mention that create_file actually writes to a temp file on the local fs first and then uploads the file to the target fs -- To view, visit http://gerrit.cloudera.org:8080/13020 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3c748529a494bb6e70fec96dc031523ff79bf61d Gerrit-Change-Number: 13020 Gerrit-PatchSet: 4 Gerrit-Owner: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: David Knupp <dkn...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Laszlo Gaal <laszlo.g...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Philip Zeyliger <phi...@cloudera.com> Gerrit-Reviewer: Sahil Takiar <stak...@cloudera.com> Gerrit-Comment-Date: Thu, 16 May 2019 20:41:12 +0000 Gerrit-HasComments: Yes
