HADOOP-16109. Parquet reading S3AFileSystem causes EOF #539
Conversation
| public ITestS3AContractSeek(final String seekPolicy) { | ||
| this.seekPolicy = seekPolicy; | ||
| } | ||
|
|
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
hadoop-common test run => failure. hadoop-aws compilation failure with a file which has been deleted |
|
💔 -1 overall
This message was automatically generated. |
|
OK, fun little Yetus issue for @aw-was-here
|
|
@steveloughran , does Yetus perhaps try to test each commit individually? Since the file is being created in the first commit and deleted in the second commit, it might be worth squashing this PR and seeing if Yetus likes that better. [Later Edit] It looks like maybe a GitHub issue. Taking a fresh clone and doing: results in the presence of an untracked not-empty ITestS3AContractRandomSeek.java file: |
mattf-apache
left a comment
mattf-apache
left a comment
There was a problem hiding this comment.
@steveloughran , thanks so much for creating all these test cases. They really do what's needed. I did request one additional case, at the end.
The other comments are mostly editorial nits on internal documentation. Please forgive :-)
There was a problem hiding this comment.
the parameterized and test settings are ... what?
Also nit: readahead not readhead
There was a problem hiding this comment.
Document allowed or intended values of seekPolicy?
There was a problem hiding this comment.
they are defined in the Constants.INPUT_FADV. vars
There was a problem hiding this comment.
Suggest moving testReadAcrossReadahead and its Javadoc down to just above testReadSingleByteAcrossReadahead, and perhaps edit Javadoc to say
* Tests for HADOOP-16109: Parquet reading S3AFileSystem causes EOF.
* The next four test cases assure correct behavior when seeks and reads
* touch or cross the end of the active S3A readahead buffer.
There was a problem hiding this comment.
Would be nice to have a describe() here too.
There was a problem hiding this comment.
will do. If you've noticed, these get logged, so help you separate "setup/teardown noise" from the test run itself.
There was a problem hiding this comment.
Strictly speaking, this is "read from immediately after boundary"
There was a problem hiding this comment.
Nice, this is definitely one of the major cases of concern.
There was a problem hiding this comment.
Yes, important test case. Could we also add a case that does:
try (FSDataInputStream in = fs.open(path)) {
readAtEndAndReturn(in);
final byte[] temp = new byte[1];
int offset = READAHEAD;
in.seek(offset);
// expect to read a byte successfully.
temp[0] = in.readByte();
assertDatasetEquals(READAHEAD, "read at end of boundary", temp, 1);
LOG.info("Read of byte at offset {} returned expected value", offset);
}
There was a problem hiding this comment.
well ok, though as readByte is read + an assert that the value >= 0, you don't really gain much.
There was a problem hiding this comment.
You're right, I guess I was thinking of readFully(), which in S3AInputStream.java can cause a new seek and thereby change the cache boundaries. You're right it's not relevant here.
There was a problem hiding this comment.
removing the comment to avoid confusion
|
new patch: address core comments from @mattf-apache , including adding a new test case. Matt, if you ever look at public final byte readByte() throws IOException {
int ch = in.read();
if (ch < 0)
throw new EOFException();
return (byte)(ch);
}the first test was effectively doing it. But in case something in future did ever subclass it, I've added the new test case |
|
tested: S3 ireland |
mattf-apache
left a comment
mattf-apache
left a comment
There was a problem hiding this comment.
Great stuff, thanks @steveloughran !
|
💔 -1 overall
This message was automatically generated. |
|
TL;DR: squash and rebase regularly
Yup. Yetus gets the equivalent of 'git format-patch' and then calls 'git apply' or 'patch' or whatever works on top of master (or branch-2 or whatever) with it. This has several downsides, but most of them go away if the patch branch is regularly rebased and commits are squashed. The latter fixes 99% of the issues. When commits aren't squashed, you'll see weird stuff like what shows up here. (Remember, the top of the other tree is moving too....) It's important to note that git/github really only provides three ways to handle PRs:
The first one has lots of issues from a Yetus functionality perspective since it taints the source tree and doing multiple checkouts has other issues (remember: yetus also runs locally!). The second one doesn't work with binary files. That leaves us with the third one, which has lots of weird idiosyncracies but works when good branch hygiene is in play. YETUS-724 will enable test-patch to switch to using the git diff IFF the format-patch version of the file can't be applied. |
Nobody gets seek right. No matter how many times they think they have. Reproducible test from: Dave Christianson Fixed seek() logic: Steve Loughran Change-Id: I4b08036b9e71e2a3c23b9da697bfc23e279ce44b
steveloughran
force-pushed
the
s3/HADOOP-16109-parquet-eof-s3a-seek
branch
from
1afe7ab to
cc41761
Compare
|
squash and resubmit |
|
🎊 +1 overall
This message was automatically generated. |
|
Squashed PR is happy. I need to do the full local test run and declare that before I can say this is ready to go in |
|
local test w/ s3 london + (s3guard). Failure in test setup of all three the big seek tests, even when parameterized names were used. Not directly related, but I'll change the creation to overwrite=true to ensure this goes away |
|
the seek contract test in this PR does set overwrite=true; build must have picked up a version of the hadoop-common tests jar from a different branch. A full clean build fixed the test run. @mattf-apache you happy with this? You have voting rights.... |
|
thanks. For the record, this was applied via dev-support/bin/smart-apply-patch --project=hadoop GH:539I'll be submitting a followup PR for branch-3.2 and earlier (doesn't compile due to no method removeBucketOverrides(java.lang.String,org.apache.hadoop.conf.Configuration,java.lang.String,java.lang.String)) which I'll have to pull in too, then all the way back to 2.8.x. Joy. This is is the price of getting seek wrong |
|
Thanks, @steveloughran ! |
…ns in KeyValueStorageEngine Author: Prateek Maheshwari <pmaheshwari@linkedin.com> Reviewers: Cameron Lee <calee@linkedin.com>, Shanthoosh Venkatraman <svenkatr@linkedin.com> Closes apache#539 from prateekm/store-metrics
4 participants
HADOOP-16109. Parquet reading S3AFileSystem causes EOF
Nobody gets seek right. No matter how many times they think they have.
Reproducible test from: Dave Christianson
Fixed seek() logic: Steve Loughran