HIVE-24669: Improve FileSystem usage in Hive::loadPartitionInternal #1893
Conversation
| return true; | ||
| } | ||
| path = path.getParent(); | ||
| } while (!path.equals(basePath)); |
There was a problem hiding this comment.
Is it possible that path.equals(basePath) won't ever be true? or path becomes null?
There was a problem hiding this comment.
It will fail with a nullpointer if you call it with an unrelated path, I will add the nullcheck
kgyrtkirk
added
tests unstable
tests pending
tests passed
and removed
tests pending
tests unstable
labels
kgyrtkirk
added
tests pending
tests unstable
and removed
tests passed
tests pending
labels
kgyrtkirk
added
tests passed
tests pending
tests unstable
and removed
tests pending
tests passed
labels
There was a problem hiding this comment.
This is significantly better on S3 as it will go from an O(dirs) number of LIST calls, at 500ms+/dir to 1 LIST per few hundred files; 200 on a versioned bucket, I think.
s3 and soon abfs will do async prefetch of results. If you can do any overlap of computation then for large/deep dir trees, even that list cost might be swallowed. But as that would be a more significant change, it's something to consider in future rather than do in this PR.
Do log the iterator.toString @ debug though; with the HADOOP-16830 patch in the s3a incremental iterators all print out the performance details of their network IO.
| @@ -117,6 +117,10 @@ public static long createTestFileId( | |||
| } | |||
| return result; | |||
| } | |||
| public static List<Path> listPath(final FileSystem fs, final Path path, final PathFilter filter, | |||
There was a problem hiding this comment.
- if you could process the results with any computation per record, you will get full benefits of the async page fetch offered by s3a and (soon) abfs; at 600ms a list for 200 records on s3, that's potentially 3ms/record saving
- If
listLocatedFileStatus()logged @ debug the toString() value of the iterator, the S3A FS iterator will print out its IOStats, including #of S3 list requests and min/mean/max durations.
There was a problem hiding this comment.
Thanks @steveloughran for taking a look at this. I will try to take advantage of the async prefetch feature in later PRs, it seems promising, but it would need bigger code change. I will checkout the IOStats downstream, I have seen it is already available there.
kgyrtkirk
added
tests pending
tests unstable
tests passed
and removed
tests unstable
tests pending
labels
| @@ -117,6 +117,10 @@ public static long createTestFileId( | |||
| } | |||
| return result; | |||
| } | |||
| public static List<Path> listPath(final FileSystem fs, final Path path, final PathFilter filter, | |||
| final boolean recursive) throws IOException { | |||
| return listLocatedFileStatus(fs, path, filter, recursive).stream().map(FileStatus::getPath).collect(Collectors.toList()); | |||
There was a problem hiding this comment.
formatting: put map/collect on new lines
kgyrtkirk
added
tests pending
tests unstable
tests passed
and removed
tests passed
tests pending
tests unstable
labels
What changes were proposed in this pull request?
Improve FileSystem usage in Hive::loadPartitionInternal to improve performance n S3
Why are the changes needed?
Performance improvement
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Current unit tests + local performance measurements.