Joe McDonnell 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 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/13020/2/bin/jenkins/release_cloud_resources.sh File bin/jenkins/release_cloud_resources.sh: http://gerrit.cloudera.org:8080/#/c/13020/2/bin/jenkins/release_cloud_resources.sh@29 PS2, Line 29: # This is currently only implemented for s3. > Consider asserting S3GUARD_DYNAMODB_TABLE and S3_BUCKET are non-empty... Added tests for S3_BUCKET, S3GUARD_DYNAMODB_TABLE, and S3GUARD_DYNAMODB_REGION. http://gerrit.cloudera.org:8080/#/c/13020/2/bin/jenkins/release_cloud_resources.sh@37 PS2, Line 37: aws s3 rm --recursive --quiet s3://${S3_BUCKET}${TEST_WAREHOUSE_DIR} > This line and line 33 is useful enough that we should either do set +x or e Added echo statements for these two. http://gerrit.cloudera.org:8080/#/c/13020/2/testdata/bin/load-test-warehouse-snapshot.sh File testdata/bin/load-test-warehouse-snapshot.sh: http://gerrit.cloudera.org:8080/#/c/13020/2/testdata/bin/load-test-warehouse-snapshot.sh@62 PS2, Line 62: if [[ "${S3GUARD_ENABLED}" = "true" ]]; then > May be useful to verify that S3GUARD_DYNAMODB_TABLE and S3GUARD_DYNAMODB_RE I added code to bin/impala-config.sh to require those to be set. This file sources that, so I'm relying on bin/impala-config.sh to check it. http://gerrit.cloudera.org:8080/#/c/13020/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: http://gerrit.cloudera.org:8080/#/c/13020/2/tests/common/impala_test_suite.py@177 PS2, Line 177: cls.filesystem_client = S3Client(S3_BUCKET_NAME) > I think we could coalesce this to use HDFS/Hadoop always, thereby maybe rem I think that makes sense. Switched this to use HdfsCommandLineClient("S3") unconditionally. http://gerrit.cloudera.org:8080/#/c/13020/2/tests/common/impala_test_suite.py@178 PS2, Line 178: elif IS_ABFS: : # ABFS is implemented via HDFS command line client : cls.filesystem_client = HdfsCommandLineClient("ABFS") : elif IS_ADLS: : cls.filesystem_client = ADLSClient(ADLS_STORE_NAME) > Do you know which, if any, of these are getting tested these days? Neither are routinely tested. http://gerrit.cloudera.org:8080/#/c/13020/2/tests/util/hdfs_util.py File tests/util/hdfs_util.py: http://gerrit.cloudera.org:8080/#/c/13020/2/tests/util/hdfs_util.py@149 PS2, Line 149: # This client is a wrapper around the hdfs command line. This is useful for filesystems > Use pydoc? Done http://gerrit.cloudera.org:8080/#/c/13020/2/tests/util/hdfs_util.py@170 PS2, Line 170: f = tempfile.NamedTemporaryFile(delete=False) > Caveat: I commented and then realized this class is just being moved, so ta Good point, that is cleaner http://gerrit.cloudera.org:8080/#/c/13020/2/tests/util/hdfs_util.py@181 PS2, Line 181: return True > Caveat: I commented and then realized this class is just being moved, so ta Yeah, it seems like we should care about the return code of the hadoop call. http://gerrit.cloudera.org:8080/#/c/13020/2/tests/util/hdfs_util.py@213 PS2, Line 213: fname = f['name'].split("/")[-1] > Caveat: I commented and then realized this class is just being moved, so ta Switched this to use os.path.basename(). -- 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: 2 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: Mon, 13 May 2019 21:27:12 +0000 Gerrit-HasComments: Yes
