impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Matthew Jacobs (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-5333: Add support for Impala to work with ADLS
Date Wed, 17 May 2017 22:09:00 GMT
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5333: Add support for Impala to work with ADLS
......................................................................


Patch Set 1:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/6910/1//COMMIT_MSG
Commit Message:

PS1, Line 18: This client seems to have some consistency
            : issues.
Is this a known issue? Any tickets to reference for this?


http://gerrit.cloudera.org:8080/#/c/6910/1/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

PS1, Line 774: S3
update


http://gerrit.cloudera.org:8080/#/c/6910/1/infra/python/deps/compiled-requirements.txt
File infra/python/deps/compiled-requirements.txt:

Line 35:   pycparser == 2.17
IIRC the indentation is meant to indicate dependency. In the Kudu case below, the comment
is meant to be the 'top level'. Just leave a comment above this indicating # Required for
adls


http://gerrit.cloudera.org:8080/#/c/6910/1/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

PS1, Line 61: seems
It'd be good to make a conclusive statement. I assume the consistency model should be well
defined somewhere.

nit: "so skip for it" sounds awkward


Line 156:       print self.filesystem_client.ls(
remove?


Line 304:       # The ADLS client seems to be inconsistent too. So skip for it.
same as above


http://gerrit.cloudera.org:8080/#/c/6910/1/tests/metadata/test_stale_metadata.py
File tests/metadata/test_stale_metadata.py:

PS1, Line 22: SkipIfADLS
not used


http://gerrit.cloudera.org:8080/#/c/6910/1/tests/query_test/test_insert_behaviour.py
File tests/query_test/test_insert_behaviour.py:

Line 48:   @SkipIfADLS.slow_client
why does this work on s3 but not adls?


http://gerrit.cloudera.org:8080/#/c/6910/1/tests/query_test/test_insert_parquet.py
File tests/query_test/test_insert_parquet.py:

Line 164:   @SkipIfADLS.hdfs_block_size
does this work on s3?


http://gerrit.cloudera.org:8080/#/c/6910/1/tests/util/adls_util.py
File tests/util/adls_util.py:

Line 26: class ADLSClient(BaseFilesystem):
nit: newline above this


Line 38:       f.write(file_data)
this has a return value, #bytes I think, it'd be good to check, e.g. log an error/return false
if it is less than expected.


PS1, Line 47:     # The ADLS Python client doesn't support cp() yet, so we have to download
and
            :     # reupload to the destination.
? http://azure-datalake-store.readthedocs.io/en/latest/api.html#azure.datalake.store.core.AzureDLFileSystem.cp


-- 
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: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message