Skip to content

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

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: s/doe/does

* 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
*/
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }

Copy link
Member

@ayushtkn ayushtkn Aug 9, 2022

Choose a reason for hiding this comment

The 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
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

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 &gt;=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
Expand All @@ -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 &gt;=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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,23 +20,29 @@

/**
* Standard strings to use in exception messages in filesystems
* HDFS is used as the reference source of the strings
* HDFS is used as the reference source of the strings.
*/
public class FSExceptionMessages {

/**
* The operation failed because the stream is closed: {@value}
* The operation failed because the stream is closed: {@value}.
*/
public static final String STREAM_IS_CLOSED = "Stream is closed!";

/**
* Negative offset seek forbidden : {@value}
* Negative offset seek forbidden : {@value}.
*/
public static final String NEGATIVE_SEEK =
"Cannot seek to a negative offset";
"Cannot seek to a negative offset";

/**
* Seeks : {@value}
* Negative offset read forbidden : {@value}.
*/
public static final String NEGATIVE_POSITION_READ =
"Cannot read from a negative position";

/**
* Seeks : {@value}.
*/
public static final String CANNOT_SEEK_PAST_EOF =
"Attempted to seek or read past the end of the file";
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,9 @@
import org.junit.BeforeClass;
import org.junit.Test;

import static org.apache.hadoop.fs.FSExceptionMessages.CANNOT_SEEK_PAST_EOF;
import static org.apache.hadoop.fs.FSExceptionMessages.EOF_IN_READ_FULLY;
import static org.apache.hadoop.fs.FSExceptionMessages.NEGATIVE_POSITION_READ;
import static org.apache.hadoop.fs.contract.ContractTestUtils.assertCapabilities;

public class TestCryptoStreams extends CryptoStreamsTestBase {
Expand Down Expand Up @@ -339,16 +342,16 @@ public void readFully(long position, ByteBuffer buf) throws IOException {
}

if (position > length) {
throw new IOException("Cannot read after EOF.");
throw new EOFException(CANNOT_SEEK_PAST_EOF);
}
if (position < 0) {
throw new IOException("Cannot read to negative offset.");
throw new EOFException(NEGATIVE_POSITION_READ);
}

checkStream();

if (position + buf.remaining() > length) {
throw new EOFException("Reach the end of stream.");
throw new EOFException(EOF_IN_READ_FULLY);
}

buf.put(data, (int) position, buf.remaining());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/****************************************************************
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I would also not want to change this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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);
Expand Down
Loading