Dimitris Tsirogiannis has posted comments on this change. Change subject: IMPALA-5333: Add support for Impala to work with ADLS ......................................................................
Patch Set 2: (15 comments) Nice! Mostly formatting nits. http://gerrit.cloudera.org:8080/#/c/6910/2/bin/impala-config.sh File bin/impala-config.sh: PS2, Line 243: azure_tenant_id nit: uppercase for consistency? PS2, Line 284: fi Should we have a dummy check like the one in L268 to make sure we can access ADLS with the given ids/secret? http://gerrit.cloudera.org:8080/#/c/6910/2/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java: PS2, Line 790: . If there is a jira for this you may want to add it in the comment. http://gerrit.cloudera.org:8080/#/c/6910/2/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: PS2, Line 306: fs instanceof LocalFileSystem || : fs instanceof AdlFileSystem); nit: fix indentation PS2, Line 325: a nit: remove PS2, Line 332: a nit: remove PS2, Line 483: FileSystemUtil.isLocalFileSystem(path) || : FileSystemUtil.isS3AFileSystem(path) || : FileSystemUtil.isADLFileSystem(path)); nit: indentation http://gerrit.cloudera.org:8080/#/c/6910/2/infra/python/bootstrap_virtualenv.py File infra/python/bootstrap_virtualenv.py: PS2, Line 63: Cffi huh? :) PS2, Line 221: CentOS How about Ubuntu? If there is a minimum version, we should mention that too. http://gerrit.cloudera.org:8080/#/c/6910/2/tests/common/impala_test_suite.py File tests/common/impala_test_suite.py: PS2, Line 137: if IS_S3: cls.filesystem_client = S3Client(S3_BUCKET_NAME) : elif IS_ADLS: cls.filesystem_client = ADLSClient(ADLS_STORE_NAME) nit: I think the format should be: if bla: kakak elif fsdfs: blablah http://gerrit.cloudera.org:8080/#/c/6910/2/tests/query_test/test_insert_behaviour.py File tests/query_test/test_insert_behaviour.py: PS2, Line 48: slow_client is this the right tag? Isn't this the consistency issue described earlier? Maybe we should create a different tag for this. PS2, Line 377: slow_client same here http://gerrit.cloudera.org:8080/#/c/6910/2/tests/query_test/test_nested_types.py File tests/query_test/test_nested_types.py: PS2, Line 25: from tests.common.skip import SkipIfOldAggsJoins, SkipIfIsilon, SkipIfS3, SkipIfADLS, SkipIfLocal long line http://gerrit.cloudera.org:8080/#/c/6910/2/tests/query_test/test_scanners.py File tests/query_test/test_scanners.py: PS2, Line 35: from tests.common.skip import SkipIfS3, SkipIfADLS, SkipIfIsilon, SkipIfOldAggsJoins, SkipIfLocal long line http://gerrit.cloudera.org:8080/#/c/6910/2/tests/util/adls_util.py File tests/util/adls_util.py: PS2, Line 40: f.close() I believe this is redundant but I could be wrong. -- To view, visit http://gerrit.cloudera.org:8080/6910 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic56b9988b32a330443f24c44f9cb2c80842f7542 Gerrit-PatchSet: 2 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Sailesh Mukil <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Dimitris Tsirogiannis <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
