Vihang Karajgaonkar has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/13665 )

Change subject: IMPALA-8663 : FileMetadataLoader should skip hidden and tmp 
directories
......................................................................


Patch Set 8:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/13665/8/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/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@541
PS8, Line 541: temporary directories
> the code now seems to also skip any files that match this pattern, not just
The latest patchset changed it so that it only skips temp directories now. I 
saw code in FileMetadataLoader which keeps stats related to hidden files. 
Didn't want to mess up those, since temp/hidden files in valid directories seem 
intentional and long-lived, while temp directories most are transient in most 
cases.


http://gerrit.cloudera.org:8080/#/c/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@545
PS8, Line 545: all underlying files (except which are
             :    * in the ignored directories)
> does this mean we no longer yield directories, and only yield files? does t
Actually, I added this line in the doc since I noticed that the 
RecursingIterator yields files not directories even without the patch. I 
thought that was important and hence added it here. These semantics are not 
changed after the patch.


http://gerrit.cloudera.org:8080/#/c/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@568
PS8, Line 568: isS3AFileSystem(p)
> why'd you change from isS3AFileSystem(fs) to isS3AFileSystem(p)? In the cas
this was unintentional change. Didn't realize there are two isS3AFileSystem 
methods and I used the wrong one.


http://gerrit.cloudera.org:8080/#/c/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@568
PS8, Line 568: isS3AFileSystem
> perhaps we can add a @VisibleForTesting way we can make this path get used
Sure. Do you have any suggestions on how can we do this? static methods are 
hard to mock. Are you thinking of using a test-only flag?


http://gerrit.cloudera.org:8080/#/c/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@626
PS8, Line 626: LISTING_TYPE
> style nit: enums should be named like classes (ListingType)
Done


http://gerrit.cloudera.org:8080/#/c/13665/8/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java@699
PS8, Line 699:       // if the current file is on a ignored path return early
             :       if (isIgnoredPath(fileStatus)) return;
> how does this prevent recursion into tmp dirs in the recursive listFiles ca
yeah, I realized that when one of my own tests failed on the last patch set. 
The latest patch set addresses this.



--
To view, visit http://gerrit.cloudera.org:8080/13665
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c4a22908304fe9e377d77d6c18d401c3f3294aa
Gerrit-Change-Number: 13665
Gerrit-PatchSet: 8
Gerrit-Owner: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Reviewer: Bharath Vissapragada <bhara...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-Reviewer: Vihang Karajgaonkar <vih...@cloudera.com>
Gerrit-Comment-Date: Thu, 27 Jun 2019 07:40:58 +0000
Gerrit-HasComments: Yes

Reply via email to