Joe McDonnell has posted comments on this change. ( http://gerrit.cloudera.org:8080/17121 )
Change subject: IMPALA-7712: Support Google Cloud Storage ...................................................................... Patch Set 6: Code-Review+2 (6 comments) Ok, I looked at this and compared it to the ABFS change and verified that there isn't anything that is missing. I had a couple small nits, but this makes sense to me. The only concern I would have is if IMPALA-10563 is more than just a slow down. http://gerrit.cloudera.org:8080/#/c/17121/5/be/src/runtime/io/disk-io-mgr.cc File be/src/runtime/io/disk-io-mgr.cc: http://gerrit.cloudera.org:8080/#/c/17121/5/be/src/runtime/io/disk-io-mgr.cc@186 PS5, Line 186: > Ah, sure. I thought tests/custom_cluster/test_hdfs_fd_caching.py provide th Yeah, file handle cache testing is not something that we have completely automated. We have basic cases covered, but we don't do the more intricate cases. It makes sense to push that out. http://gerrit.cloudera.org:8080/#/c/17121/6/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java: http://gerrit.cloudera.org:8080/#/c/17121/6/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@804 PS6, Line 804: hasNex Nit: hasNext() http://gerrit.cloudera.org:8080/#/c/17121/6/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@805 PS6, Line 805: hashNext Nit: hasNext() http://gerrit.cloudera.org:8080/#/c/17121/6/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@806 PS6, Line 806: hashNext Nit: hasNext() http://gerrit.cloudera.org:8080/#/c/17121/6/tests/custom_cluster/test_hdfs_fd_caching.py File tests/custom_cluster/test_hdfs_fd_caching.py: http://gerrit.cloudera.org:8080/#/c/17121/6/tests/custom_cluster/test_hdfs_fd_caching.py@132 PS6, Line 132: # Caching applies to HDFS, S3, ABFS, and GCS files. If this is HDFS, S3, ABFS or GCS, : # then verify that caching works. Otherwise, verify that file handles are not cached. Nit: Now we don't cache GCS file handles, so this needs to be updated. http://gerrit.cloudera.org:8080/#/c/17121/5/tests/stress/test_insert_stress.py File tests/stress/test_insert_stress.py: http://gerrit.cloudera.org:8080/#/c/17121/5/tests/stress/test_insert_stress.py@81 PS5, Line 81: @SkipIfGCS.jira(reason="IMPALA-10563") > Yes, it's consistently reproducable on GCE instances, even if I use a newer Ok, to be clear, the statement runs slower, but it does eventually complete correctly. Is that right? -- To view, visit http://gerrit.cloudera.org:8080/17121 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ia91ec956de3b620cccf6a1244b56b7da7a45b32b Gerrit-Change-Number: 17121 Gerrit-PatchSet: 6 Gerrit-Owner: Quanlong Huang <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Quanlong Huang <[email protected]> Gerrit-Comment-Date: Wed, 10 Mar 2021 01:24:43 +0000 Gerrit-HasComments: Yes
