fix: hdfs read into buffer fully #2031
Conversation
parthchandra
requested review from
andygrove and
comphead
and removed request for
andygrove
|
@Kontinuation, fyi |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
============================================
+ Coverage 56.12% 58.67% +2.55%
- Complexity 976 1253 +277
============================================
Files 119 134 +15
Lines 11743 13102 +1359
Branches 2251 2396 +145
============================================
+ Hits 6591 7688 +1097
- Misses 4012 4179 +167
- Partials 1140 1235 +95 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| .read_with_pos((range.start + total_read) as i64, buf[total_read as usize..].as_mut()) | ||
| .map_err(to_error)?; | ||
| if read == -1 { | ||
| break; |
There was a problem hiding this comment.
Shouldn't this return an unexpected eof error?
There was a problem hiding this comment.
Agree, otherwise the function result is probably not predictable.
There was a problem hiding this comment.
Actually this implementation is incorrect. read_with_pos never returns -1. Instead, it returns 0 when hitting EOF.
libhdfs behaves strangely, and so does fs-hdfs.
References:
There was a problem hiding this comment.
Thanks for catching this @Kontinuation! I based this on the return value of FSDataInputStream, but looks like both fs-hdfs and the official hadoop libhdfs behave differently.
Let me address this.
In fs-hdfs
: a return value < 0 is an error
: a return value of 0 is OK even if the buffer is not fully read into. However, it looks like other object store implementations treat this as an error. So I will treat both as an error (maybe reintroduce the original assertion).
native/hdfs/src/object_store/hdfs.rs
Outdated
| let read = file.read(buf.as_mut_slice()).map_err(to_error)?; | ||
| if read == -1 { | ||
| break; | ||
| } |
There was a problem hiding this comment.
This is also problematic. read also return 0 when hitting EOF and never return an negative value.
There was a problem hiding this comment.
Sorry @Kontinuation if I check your references https://github.com/datafusion-contrib/fs-hdfs/blob/8c03c5ef0942b75abc79ed673931355fa9552131/c_src/libhdfs/hdfs.c#L1564C15-L1564C19 like here, it actually returns -1?
When EOF was hit,
The if (jVal.i < 0) {
// EOF
destroyLocalReference(env, jbRarray);
return 0;
} |
|
@Kontinuation @andygrove @comphead, updated based on review comments |
| @@ -88,19 +88,33 @@ impl HadoopFileSystem { | |||
|
|
|||
| fn read_range(range: &Range<u64>, file: &HdfsFile) -> Result<Bytes> { | |||
| let to_read = (range.end - range.start) as usize; | |||
| let mut total_read = 0u64; | |||
There was a problem hiding this comment.
wondering would be that better to have total_read as usize?
There was a problem hiding this comment.
This patch looks good to me, and it reminds me another problem with fs-hdfs.
The HdfsErr returned by fs-hdfs read functions does not contain JVM stack traces. If there's a read failure caused by an exception in JVM, all we get is an Err(HdfsErr::Generic) with a vague error message. The stack trace will only be printed to stderr by libhdfs but not get propagated to the Java exception thrown by Comet. User has to browse the executor logs to know what happened when a query failure was caused by fs-hdfs read failure.
libhdfs has getLastTLSExceptionRootCause and getLastTLSExceptionStackTrace for retrieving detailed information about the exception, but fs-hdfs is not making use of them.
An ideal approach is to rethrow the original Java exception using env.throw, just like how Comet handles other JNI call exceptions. But this requires revising how Java exception objects are managed in libhdfs.
The
getandread_rangemethods in hdfs object store implementation do not always read the data requested because the underlying hdfs call may return fewer bytes than requested.#1992 (comment)