-
Notifications
You must be signed in to change notification settings - Fork 9.1k
HDFS-15042 Add more tests for ByteBufferPositionedReadable. #1747
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,9 +25,24 @@ | |
import org.apache.hadoop.classification.InterfaceStability; | ||
|
||
/** | ||
* The javadocs for this interface follows RFC 2119 rules regarding the use of | ||
* MUST, MUST NOT, MAY, and SHALL. | ||
* <p> | ||
* Implementers of this interface provide a positioned read API that writes to a | ||
* {@link ByteBuffer} rather than a {@code byte[]}. | ||
* | ||
* <p> | ||
* <b>Thread safety</b> | ||
* <p> | ||
* These operations doe not change the current offset of a stream as returned | ||
* by {@link Seekable#getPos()} and MUST BE thread-safe. | ||
* Implementations MAY block other readers while one read operation | ||
* is in progress; this MAY include blocking <i>all</i> read() calls. | ||
* <p> | ||
* <b>Concurrent file access</b> | ||
* <p> | ||
* No guarantees are made as to when or whether changes made to a file | ||
* are visible to readers of this API reading the data through an already | ||
* open stream instance. | ||
* @see PositionedReadable | ||
* @see ByteBufferReadable | ||
*/ | ||
|
@@ -54,12 +69,11 @@ public interface ByteBufferPositionedReadable { | |
* stream supports this interface, otherwise they might get a | ||
* {@link UnsupportedOperationException}. | ||
* <p> | ||
* Implementations should treat 0-length requests as legitimate, and must not | ||
* Implementations MUST treat 0-length requests as legitimate, and MUST NOT | ||
* signal an error upon their receipt. | ||
* <p> | ||
* This does not change the current offset of a file, and is thread-safe. | ||
* | ||
* @param position position within file | ||
* The {@code position} offset MUST BE zero or positive; if negative | ||
* an EOFException SHALL BE raised. | ||
Comment on lines
+74
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If DFSInputStream:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
* @param position position within stream. This MUST be >=0. | ||
* @param buf the ByteBuffer to receive the results of the read operation. | ||
* @return the number of bytes read, possibly zero, or -1 if reached | ||
* end-of-stream | ||
|
@@ -78,12 +92,23 @@ public interface ByteBufferPositionedReadable { | |
* {@link #read(long, ByteBuffer)}, the difference is that this method is | ||
* guaranteed to read data until the {@link ByteBuffer} is full, or until | ||
* the end of the data stream is reached. | ||
* | ||
* @param position position within file | ||
* <p> | ||
* The {@code position} offset MUST BE zero or positive; if negative | ||
* an EOFException SHALL BE raised. | ||
* <p> | ||
* Implementations MUST treat 0-length requests as legitimate -and, and so | ||
* MUST NOT signal an error if the read position was valid. | ||
* For zero-byte reads, read position greater {@code getPos()} MAY be | ||
* considered valid; a negative position MUST always fail. | ||
* <p> | ||
* If the EOF was reached before the buffer was completely read -the | ||
* state of the buffer is undefined. It may be unchanged or it may be | ||
* partially or completely overwritten. | ||
* @param position position within stream. This must be >=0. | ||
* @param buf the ByteBuffer to receive the results of the read operation. | ||
* @throws IOException if there is some error performing the read | ||
* @throws EOFException the end of the data was reached before | ||
* the read operation completed | ||
* the read operation completed, or the position value is negative. | ||
* @see #read(long, ByteBuffer) | ||
*/ | ||
void readFully(long position, ByteBuffer buf) throws IOException; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,6 +94,7 @@ | |
|
||
import javax.annotation.Nonnull; | ||
|
||
import static org.apache.hadoop.fs.FSExceptionMessages.NEGATIVE_POSITION_READ; | ||
import static org.apache.hadoop.hdfs.util.IOUtilsClient.updateReadStatistics; | ||
|
||
/**************************************************************** | ||
|
@@ -1684,6 +1685,9 @@ public int read(long position, final ByteBuffer buf) throws IOException { | |
@Override | ||
public void readFully(long position, final ByteBuffer buf) | ||
throws IOException { | ||
if (position < 0) { | ||
throw new EOFException(NEGATIVE_POSITION_READ); | ||
} | ||
Comment on lines
+1688
to
+1690
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The exception message will change now, earlier it would be from below
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I would also not want to change this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok |
||
int nread = 0; | ||
while (buf.hasRemaining()) { | ||
int nbytes = read(position + nread, buf); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/doe/does