JoshRosen commented on a change in pull request #24672: [SPARK-27801]
InMemoryFileIndex.listLeafFiles should use listLocatedStatus for
DistributedFileSystem
URL: https://github.com/apache/spark/pull/24672#discussion_r286329259
##########
File path:
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/InMemoryFileIndex.scala
##########
@@ -274,7 +275,21 @@ object InMemoryFileIndex extends Logging {
// [SPARK-17599] Prevent InMemoryFileIndex from failing if path doesn't
exist
// Note that statuses only include FileStatus for the files and dirs
directly under path,
// and does not include anything else recursively.
- val statuses = try fs.listStatus(path) catch {
+ val statuses: Array[FileStatus] = try {
+ fs match {
+ // DistributedFileSystem overrides listLocatedStatus to make 1 single
call to namenode
+ // to retrieve the file status with the file block location. The
reason to still fallback
+ // to listStatus is because the default implementation would
potentially throw a
+ // FileNotFoundException which is better handled by doing the lookups
manually below.
Review comment:
Ah, I think I get it now: I was originally confused by the comment, but on
re-read it sounds like you're saying that we _could_, in principle, just call
`listLocatedStatus()` in all cases, except that there's no performance benefit
for non `DistributedFileSystem` subclasses and we lose more granular error
handling.
In fact, the granular error handling could be really important in order to
preserve the behavior before my changes in #24668 (a.k.a. the new behavior when
`ignoreMissingFiles = true`): previously, the deletion of a leaf file after a
directory listing but before listing the file status would cause _only that
file_ to be ignored, whereas using `listLocatedStatus` for
non-`DistributedFileSystem` subclasses would allow that `FileNotFoundException`
to trigger the dropping of _all_ files from the listing (even ones whose
statuses could be retrieved).
I feel like `DistributedFileSystem` must be having some sort of internal
"atomicity"-type guarantee for this to be safe, though (that it if it sees a
file then it's able to also see its status).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
