HDFS-15042 Add more tests for ByteBufferPositionedReadable. #1747
+339
−99
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
hdfs failures clearly something else will rebase and fix checkstyles anyway |
|
@sahilTakiar fyi |
steveloughran
force-pushed
the
filesystem/HDFS-15042-ByteBufferPositionedReadable
branch
from
067e0a0 to
79f8583
Compare
|
Again, hadoop-hdfs test failures must be unrelated: this patch doesn't touch those tests |
|
steveloughran
force-pushed
the
filesystem/HDFS-15042-ByteBufferPositionedReadable
branch
from
79f8583 to
44494c7
Compare
steveloughran
force-pushed
the
filesystem/HDFS-15042-ByteBufferPositionedReadable
branch
from
44494c7 to
ab7c180
Compare
steveloughran
force-pushed
the
filesystem/HDFS-15042-ByteBufferPositionedReadable
branch
from
ab7c180 to
6e050d9
Compare
|
💔 -1 overall
This message was automatically generated. |
|
both failures are unrelated...will rebase. test failure is in javadoc failure |
Changes * Javadocs in ByteBufferPositionedReadable are a bit stricter. * switch to parameterized JUnit test runs for on/off heap buffers * factor put main assertions into higher level asserts New tests * negative offset for reads of actual/empty buffers * read position > EOF. * empty files * verify that interleaving simple reads/seeks work. Change-Id: I27294b15f7cad8e1b87150d9e9fac0623fc44c45
Change-Id: I4c8b17dd897a11d4c8e22b9cbc366455c958242c
steveloughran
force-pushed
the
filesystem/HDFS-15042-ByteBufferPositionedReadable
branch
from
6e050d9 to
ac0bcd8
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
@mukund-thakur @mehakmeet @virajjasani could i get a quick review of this? i want these tests in before anyone tries to implement it in s3a/abfs/gcs via vectored io |
virajjasani
reviewed
Aug 8, 2022
| * <p> | ||
| * <b>Thread safety</b> | ||
| * <p> | ||
| * These operations doe not change the current offset of a stream as returned |
Comment on lines
+74
to
+75
| * The {@code position} offset MUST BE zero or positive; if negative | ||
| * an EOFException SHALL BE raised. |
There was a problem hiding this comment.
If position is negative, this method seems to be returning -1 instead of throwing EOF with NEGATIVE_POSITION_READ reason. EOF with NEGATIVE_POSITION_READ is being thrown with this patch only with void readFully(long position, ByteBuffer buf) method.
DFSInputStream:
private int pread(long position, ByteBuffer buffer)
throws IOException {
// sanity checks
dfsClient.checkOpen();
if (closed.get()) {
throw new IOException("Stream closed");
}
failures = 0;
long filelen = getFileLength();
if ((position < 0) || (position >= filelen)) {
return -1;
}
There was a problem hiding this comment.
Just to be clear, we can't change the behaviour of the stream as well, we just need to correct the JavaDoc, changing behaviour to now throw EOF will be incompatible
The below Javadoc also marks -1 as a legitimate return value
* @return the number of bytes read, possibly zero, or -1 if reached
* end-of-stream
There was a problem hiding this comment.
well, i think that's broken. like you say, can't fix though. or we could, but that complicates code written against the eof raising impl running against older/external stuff
ayushtkn
reviewed
Aug 9, 2022
Comment on lines
+1688
to
+1690
| if (position < 0) { | ||
| throw new EOFException(NEGATIVE_POSITION_READ); | ||
| } |
There was a problem hiding this comment.
The exception message will change now, earlier it would be from below
if (nbytes < 0) {
throw new EOFException(FSExceptionMessages.EOF_IN_READ_FULLY);
}
Someone asserting on the exception message will get issues, not sure if it is something to do in scope of adding new tests
There was a problem hiding this comment.
Yeah I would also not want to change this.
| */ | ||
| private void assertStreamPosition(final Seekable in, long pos) | ||
| throws IOException { | ||
| assertEquals("Buffer position", |
Comment on lines
+418
to
+422
| if (buffer.position() > 0) { | ||
| // this implementation does not do a range check before the read; | ||
| // it got partway through before failing. | ||
| // this is not an error -just inefficient. | ||
| LOG.warn("Buffer reads began before range checks with {}", in); |
There was a problem hiding this comment.
What is the benefit of having this, is this log gonna help sometime is debugging?
There was a problem hiding this comment.
i really don't remember. this is a pr from 2019.
|
🎊 +1 overall
This message was automatically generated. |
|
Forgotten about this. #6686 would benefit from this |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
New tests
Change-Id: I27294b15f7cad8e1b87150d9e9fac0623fc44c45