Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/17121 )

Change subject: IMPALA-7712: Support Google Cloud Storage
......................................................................


Patch Set 10: Code-Review+2

(1 comment)

Given the analysis in IMPALA-10563, it seems fine to disable those test cases 
for now.

See my note about IMPALA-10579. I think it is ok to include this partial fix, 
as it seems better than what we have right now. If IMPALA-10579 was landing 
very soon, I would be ok with removing this piece of the fix and relying on 
IMPALA-10579.

This change makes sense to me, and it is good to have the GCS support land.

http://gerrit.cloudera.org:8080/#/c/17121/10/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/10/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@713
PS10, Line 713:   /**
              :    * Wrapper around FileSystem.listStatusIterator() to make 
sure the path exists.
              :    *
              :    * @throws FileNotFoundException if <code>p</code> does not 
exist
              :    * @throws IOException if any I/O error occurredd
              :    */
              :   public static RemoteIterator<FileStatus> 
listStatusIterator(FileSystem fs, Path p)
              :       throws IOException {
              :     RemoteIterator<FileStatus> iterator = 
fs.listStatusIterator(p);
              :     // Some FileSystem implementations like 
GoogleHadoopFileSystem doesn't check
              :     // existence of the start path when creating the 
RemoteIterator. Instead, their
              :     // iterators throw the FileNotFoundException in the first 
call of hasNext() when
              :     // the start path doesn't exist. Here we call hasNext() to 
ensure start path exists.
              :     iterator.hasNext();
              :     return iterator;
This code will be replaced by IMPALA-10579.

I'm guessing that the thought here is that this is better than what we have, 
and the fuller fix will come from IMPALA-10579.



--
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: 10
Gerrit-Owner: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com>
Gerrit-Reviewer: Quanlong Huang <huangquanl...@gmail.com>
Gerrit-Comment-Date: Fri, 12 Mar 2021 18:42:52 +0000
Gerrit-HasComments: Yes

Reply via email to