GH-3078: Use Hadoop FileSystem.openFile() to open files #3079
Conversation
* Open files with FileSystem.openFile(), passing in file status * And read policy of "parquet, vector, random, adaptive"
steveloughran
force-pushed
the
GH-3078-openfile
branch
from
b1b490b to
ab31f8f
Compare
|
@Fokko @gszadovszky Could you help review this? Thanks! |
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/util/wrapped/io/FutureIO.java
Show resolved
Hide resolved
| // equal the path in the FileStatus; Hive virtual FS could create this condition. | ||
| // As the path to open is derived from stat.getPath(), this condition seems | ||
| // near-impossible to create -but is handled here for due diligence. | ||
| stream = fs.open(stat.getPath()); |
There was a problem hiding this comment.
Shouldn't we at least log the original exception?
There was a problem hiding this comment.
good point. at debug? I'm never a fan of excessively noisy libraries as they ruin the lives of people downstream, especially if messages for developers end up being printed in user logs far too often. In fact, I'm not above dynamically patching log levels to shut those libraries up (HADOOP-19272)...
stack trace then, debug.
error text itself? Probably the same unless there's a desire to log exactly once during the life of a process, which could be done trivially
There was a problem hiding this comment.
I'm fine with any solutions just did not want to loose the exception may occur.
Another idea I've had is to catch the potential exception may come out of this line (let's call it e2) and add the previously caught e as suppressed to e2, then re-throw. But it might be an overkill since this code path "near-impossible" to be taken.
There was a problem hiding this comment.
The main issue we hit is that
- filestatus includes full path
- hive's wrapper fs has to create new fs instances with the wrapper fs path
- and s3a was checking the source status matched that of the target path, raising IllegalArgumentException
Hadoop 3.3.5+ only checks filename now
How about
- log at debug
- add as suppressed.
Exceptions are too important to lose after all.
|
I'm going to propose the following tests to simulate failure and validate handling. openFile() raises IllegalArgumentException, open() works.open() succeeds so stream returned. openFile() raises IllegalArgumentException, open() raises IOEIOE thrown with IllegalArgumentException as suppressed openFile() returns a Future, whose get() raises an UncheckedIOEunchecked IOE unwrapped and thrown. openFile() returns a stream, open() raises IOEstream returned, showing open() wasn't invoked. Does that cover the codepaths? |
|
At the last scenario, the "open() raises IOE" doesn't matter, right? I mean, it can be a test coding detail to throw an exception there so we are sure open() is not invoked, but this is not part of the actual scenario. (We might use Mockito to check if the method was invoked or not, no need for an exception.) Otherwise, yes, I believe, these are all the scenarios might happen in this code. |
Good point, and yes same codepath. My feelings about mockito are mixed fwiw; tests can be very brittle and often have high maintenance costs. As this is public API stuff, not only shouldn't it be an issue -if it does fail, that's a regression. |
The builder API is a real PITA to mock, even though it works great in application code. Uses the o.a.h.fs.impl implementation, and I've created HADOOP-19355. Mark FutureDataInputStreamBuilderImpl as VisibleForTesting to make it clear that this is allowed.
|
shaves a HEAD request! for s3a it tells things to seek properly rather than having to guess afterwards. FWIW there's a "whole-file" read policy, we use this in hadoop itself for stuff like distcp...now I need to get the sequential policy into avro so it knows that prefetching is good, rather than a waste of IO capacity |
Rationale for this change
If parquet passes down the
FileStatusit previously collected, S3A and ABFS clients can skip their own HEAD probe. This saves time, money and IO capacity.When the read policy is passed in, the clients know immediately what kind of read pattern to expect, rather than infer it as reads take place or from the file suffix.
What changes are included in this PR?
Are these changes tested?
Through parquet-hadoop.
Are there any user-facing changes?
no.
Closes #3078