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

Reply via email to