[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[ https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17853767#comment-17853767 ] ASF GitHub Bot commented on HDFS-15042: --- steveloughran commented on PR #1747: URL: https://github.com/apache/hadoop/pull/1747#issuecomment-2159004833 Forgotten about this. #6686 would benefit from this > Add more tests for ByteBufferPositionedReadable > > > Key: HDFS-15042 > URL: https://issues.apache.org/jira/browse/HDFS-15042 > Project: Hadoop HDFS > Issue Type: Improvement > Components: fs, test >Affects Versions: 3.3.0 >Reporter: Steve Loughran >Assignee: Steve Loughran >Priority: Major > Labels: pull-request-available > Time Spent: 1.5h > Remaining Estimate: 0h > > There's a few corner cases of ByteBufferPositionedReadable which need to be > tested, mainly illegal read positions. Add them -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[ https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17802422#comment-17802422 ] Shilun Fan commented on HDFS-15042: --- updated the target version for preparing 3.4.0 release. > Add more tests for ByteBufferPositionedReadable > > > Key: HDFS-15042 > URL: https://issues.apache.org/jira/browse/HDFS-15042 > Project: Hadoop HDFS > Issue Type: Improvement > Components: fs, test >Affects Versions: 3.3.0 >Reporter: Steve Loughran >Assignee: Steve Loughran >Priority: Major > Labels: pull-request-available > Time Spent: 1.5h > Remaining Estimate: 0h > > There's a few corner cases of ByteBufferPositionedReadable which need to be > tested, mainly illegal read positions. Add them -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17744741#comment-17744741
]
ASF GitHub Bot commented on HDFS-15042:
---
mukund-thakur commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r956411939
##
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java:
##
@@ -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);
+}
Review Comment:
Yeah I would also not want to change this.
> Add more tests for ByteBufferPositionedReadable
>
>
> Key: HDFS-15042
> URL: https://issues.apache.org/jira/browse/HDFS-15042
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: fs, test
>Affects Versions: 3.3.0
>Reporter: Steve Loughran
>Assignee: Steve Loughran
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> There's a few corner cases of ByteBufferPositionedReadable which need to be
> tested, mainly illegal read positions. Add them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17744709#comment-17744709
]
ASF GitHub Bot commented on HDFS-15042:
---
steveloughran commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r1268335189
##
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestByteBufferPread.java:
##
@@ -161,130 +229,264 @@ private void testPreadWithFullByteBuffer(ByteBuffer
buffer)
* {@link ByteBuffer#limit()} on the buffer. Validates that only half of the
* testFile is loaded into the buffer.
*/
- private void testPreadWithLimitedByteBuffer(
- ByteBuffer buffer) throws IOException {
+ @Test
+ public void testPreadWithLimitedByteBuffer() throws IOException {
int bytesRead;
int totalBytesRead = 0;
// Set the buffer limit to half the size of the file
-buffer.limit(FILE_SIZE / 2);
+buffer.limit(HALF_SIZE);
try (FSDataInputStream in = fs.open(testFile)) {
+ in.seek(EOF_POS);
while ((bytesRead = in.read(totalBytesRead, buffer)) > 0) {
totalBytesRead += bytesRead;
// Check that each call to read changes the position of the ByteBuffer
// correctly
-assertEquals(totalBytesRead, buffer.position());
+assertBufferPosition(totalBytesRead);
}
// Since we set the buffer limit to half the size of the file, we should
// have only read half of the file into the buffer
- assertEquals(totalBytesRead, FILE_SIZE / 2);
+ assertEquals(HALF_SIZE, totalBytesRead);
// Check that the buffer is full and the contents equal the first half of
// the file
- assertFalse(buffer.hasRemaining());
- buffer.position(0);
- byte[] bufferContents = new byte[FILE_SIZE / 2];
- buffer.get(bufferContents);
- assertArrayEquals(bufferContents,
- Arrays.copyOfRange(fileContents, 0, FILE_SIZE / 2));
+ assertBufferIsFull();
+ assertBufferEqualsFileContents(0, HALF_SIZE, 0);
+
+ // position hasn't changed
+ assertStreamPosition(in, EOF_POS);
}
}
/**
* Reads half of the testFile into the {@link ByteBuffer} by setting the
* {@link ByteBuffer#position()} the half the size of the file. Validates
that
* only half of the testFile is loaded into the buffer.
+ *
+ * This test interleaves reading from the stream by the classic input
+ * stream API, verifying those bytes are also as expected.
+ * This lets us validate the requirement that these positions reads must
+ * not interfere with the conventional read sequence.
*/
- private void testPreadWithPositionedByteBuffer(
- ByteBuffer buffer) throws IOException {
+ @Test
+ public void testPreadWithPositionedByteBuffer() throws IOException {
int bytesRead;
int totalBytesRead = 0;
// Set the buffer position to half the size of the file
-buffer.position(FILE_SIZE / 2);
+buffer.position(HALF_SIZE);
+int counter = 0;
try (FSDataInputStream in = fs.open(testFile)) {
+ assertEquals("Byte read from stream",
+ fileContents[counter++], in.read());
while ((bytesRead = in.read(totalBytesRead, buffer)) > 0) {
totalBytesRead += bytesRead;
// Check that each call to read changes the position of the ByteBuffer
// correctly
-assertEquals(totalBytesRead + FILE_SIZE / 2, buffer.position());
+assertBufferPosition(totalBytesRead + HALF_SIZE);
+// read the next byte.
+assertEquals("Byte read from stream",
+fileContents[counter++], in.read());
}
// Since we set the buffer position to half the size of the file, we
// should have only read half of the file into the buffer
- assertEquals(totalBytesRead, FILE_SIZE / 2);
+ assertEquals("bytes read",
+ HALF_SIZE, totalBytesRead);
// Check that the buffer is full and the contents equal the first half of
// the file
- assertFalse(buffer.hasRemaining());
- buffer.position(FILE_SIZE / 2);
- byte[] bufferContents = new byte[FILE_SIZE / 2];
- buffer.get(bufferContents);
- assertArrayEquals(bufferContents,
- Arrays.copyOfRange(fileContents, 0, FILE_SIZE / 2));
+ assertBufferIsFull();
+ assertBufferEqualsFileContents(HALF_SIZE, HALF_SIZE, 0);
}
}
+ /**
+ * Assert the buffer ranges matches that in the file.
+ * @param bufferPosition buffer position
+ * @param length length of data to check
+ * @param fileOffset offset in file.
+ */
+ private void assertBufferEqualsFileContents(int bufferPosition,
+ int length,
+ int fileOffset) {
+buffer.position(bufferPosition);
+byte[] bufferContents = new byte[length];
+buffer.get(bufferContents);
+assertArrayEquals(
+"Buffer data from [" +
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17744708#comment-17744708
]
ASF GitHub Bot commented on HDFS-15042:
---
steveloughran commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r1268334625
##
hadoop-hdfs-project/hadoop-hdfs-client/src/main/java/org/apache/hadoop/hdfs/DFSInputStream.java:
##
@@ -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);
+}
Review Comment:
ok
> Add more tests for ByteBufferPositionedReadable
>
>
> Key: HDFS-15042
> URL: https://issues.apache.org/jira/browse/HDFS-15042
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: fs, test
>Affects Versions: 3.3.0
>Reporter: Steve Loughran
>Assignee: Steve Loughran
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> There's a few corner cases of ByteBufferPositionedReadable which need to be
> tested, mainly illegal read positions. Add them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17743791#comment-17743791
]
ASF GitHub Bot commented on HDFS-15042:
---
hadoop-yetus commented on PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#issuecomment-1638056754
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 58s | | Docker mode activated. |
_ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files
found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available.
|
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain
any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to
include 2 new or modified test files. |
_ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 44s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 31m 24s | | trunk passed |
| +1 :green_heart: | compile | 17m 21s | | trunk passed with JDK
Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | compile | 16m 2s | | trunk passed with JDK
Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | checkstyle | 4m 27s | | trunk passed |
| +1 :green_heart: | mvnsite | 4m 57s | | trunk passed |
| +1 :green_heart: | javadoc | 3m 49s | | trunk passed with JDK
Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 3m 59s | | trunk passed with JDK
Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 9m 0s | | trunk passed |
| +1 :green_heart: | shadedclient | 37m 55s | | branch has no errors
when building and testing our client artifacts. |
_ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 33s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 5s | | the patch passed |
| +1 :green_heart: | compile | 18m 49s | | the patch passed with JDK
Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javac | 18m 49s | | the patch passed |
| +1 :green_heart: | compile | 18m 14s | | the patch passed with JDK
Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | javac | 18m 14s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks
issues. |
| +1 :green_heart: | checkstyle | 4m 16s | | root: The patch generated
0 new + 45 unchanged - 5 fixed = 45 total (was 50) |
| +1 :green_heart: | mvnsite | 4m 46s | | the patch passed |
| +1 :green_heart: | javadoc | 3m 49s | | the patch passed with JDK
Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1 |
| +1 :green_heart: | javadoc | 4m 0s | | the patch passed with JDK
Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| +1 :green_heart: | spotbugs | 9m 47s | | the patch passed |
| +1 :green_heart: | shadedclient | 39m 51s | | patch has no errors
when building and testing our client artifacts. |
_ Other Tests _ |
| +1 :green_heart: | unit | 19m 18s | | hadoop-common in the patch
passed. |
| +1 :green_heart: | unit | 2m 53s | | hadoop-hdfs-client in the patch
passed. |
| +1 :green_heart: | unit | 254m 44s | | hadoop-hdfs in the patch
passed. |
| +1 :green_heart: | asflicense | 1m 25s | | The patch does not
generate ASF License warnings. |
| | | 533m 51s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-1747/1/artifact/out/Dockerfile
|
| GITHUB PR | https://github.com/apache/hadoop/pull/1747 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall
mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux d5d1eca41bff 4.15.0-212-generic #223-Ubuntu SMP Tue May 23
13:09:22 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / ac0bcd8e379f186a5d244284ef4ffcd640c89ed6 |
| Default Java | Private Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Multi-JDK versions |
/usr/lib/jvm/java-11-openjdk-amd64:Ubuntu-11.0.19+7-post-Ubuntu-0ubuntu120.04.1
/usr/lib/jvm/java-8-openjdk-amd64:Private
Build-1.8.0_362-8u372-ga~us1-0ubuntu1~20.04-b09 |
| Test Results |
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-1747/1/testReport/ |
| Max. process+thread count | 3023 (vs. ulimit
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17577553#comment-17577553
]
ASF GitHub Bot commented on HDFS-15042:
---
steveloughran commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r941634758
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ByteBufferPositionedReadable.java:
##
@@ -54,12 +69,11 @@ public interface ByteBufferPositionedReadable {
* stream supports this interface, otherwise they might get a
* {@link UnsupportedOperationException}.
*
- * 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.
- *
- * 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.
Review 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
> Add more tests for ByteBufferPositionedReadable
>
>
> Key: HDFS-15042
> URL: https://issues.apache.org/jira/browse/HDFS-15042
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: fs, test
>Affects Versions: 3.3.0
>Reporter: Steve Loughran
>Assignee: Steve Loughran
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> There's a few corner cases of ByteBufferPositionedReadable which need to be
> tested, mainly illegal read positions. Add them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17577204#comment-17577204
]
ASF GitHub Bot commented on HDFS-15042:
---
ayushtkn commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r940946727
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ByteBufferPositionedReadable.java:
##
@@ -54,12 +69,11 @@ public interface ByteBufferPositionedReadable {
* stream supports this interface, otherwise they might get a
* {@link UnsupportedOperationException}.
*
- * 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.
- *
- * 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.
Review 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
```
##
hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestByteBufferPread.java:
##
@@ -161,130 +229,264 @@ private void testPreadWithFullByteBuffer(ByteBuffer
buffer)
* {@link ByteBuffer#limit()} on the buffer. Validates that only half of the
* testFile is loaded into the buffer.
*/
- private void testPreadWithLimitedByteBuffer(
- ByteBuffer buffer) throws IOException {
+ @Test
+ public void testPreadWithLimitedByteBuffer() throws IOException {
int bytesRead;
int totalBytesRead = 0;
// Set the buffer limit to half the size of the file
-buffer.limit(FILE_SIZE / 2);
+buffer.limit(HALF_SIZE);
try (FSDataInputStream in = fs.open(testFile)) {
+ in.seek(EOF_POS);
while ((bytesRead = in.read(totalBytesRead, buffer)) > 0) {
totalBytesRead += bytesRead;
// Check that each call to read changes the position of the ByteBuffer
// correctly
-assertEquals(totalBytesRead, buffer.position());
+assertBufferPosition(totalBytesRead);
}
// Since we set the buffer limit to half the size of the file, we should
// have only read half of the file into the buffer
- assertEquals(totalBytesRead, FILE_SIZE / 2);
+ assertEquals(HALF_SIZE, totalBytesRead);
// Check that the buffer is full and the contents equal the first half of
// the file
- assertFalse(buffer.hasRemaining());
- buffer.position(0);
- byte[] bufferContents = new byte[FILE_SIZE / 2];
- buffer.get(bufferContents);
- assertArrayEquals(bufferContents,
- Arrays.copyOfRange(fileContents, 0, FILE_SIZE / 2));
+ assertBufferIsFull();
+ assertBufferEqualsFileContents(0, HALF_SIZE, 0);
+
+ // position hasn't changed
+ assertStreamPosition(in, EOF_POS);
}
}
/**
* Reads half of the testFile into the {@link ByteBuffer} by setting the
* {@link ByteBuffer#position()} the half the size of the file. Validates
that
* only half of the testFile is loaded into the buffer.
+ *
+ * This test interleaves reading from the stream by the classic input
+ * stream API, verifying those bytes are also as expected.
+ * This lets us validate the requirement that these positions reads must
+ * not interfere with the conventional read sequence.
*/
- private void testPreadWithPositionedByteBuffer(
- ByteBuffer buffer) throws IOException {
+ @Test
+ public void testPreadWithPositionedByteBuffer() throws IOException {
int bytesRead;
int totalBytesRead = 0;
// Set the buffer position to half the size of the file
-buffer.position(FILE_SIZE / 2);
+buffer.position(HALF_SIZE);
+int counter = 0;
try (FSDataInputStream in = fs.open(testFile)) {
+ assertEquals("Byte read from stream",
+ fileContents[counter++], in.read());
while ((bytesRead = in.read(totalBytesRead, buffer)) > 0) {
totalBytesRead += bytesRead;
// Check that each call to read changes the position of the ByteBuffer
// correctly
-assertEquals(totalBytesRead + FILE_SIZE / 2, buffer.position());
+assertBufferPosition(totalBytesRead + HALF_SIZE);
+// read the next byte.
+assertEquals("Byte read from stream",
+fileContents[counter++], in.read());
}
// Since we set the buffer position to half the size of the file, we
// should have only read half of the file
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17577187#comment-17577187
]
ASF GitHub Bot commented on HDFS-15042:
---
ayushtkn commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r940946727
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ByteBufferPositionedReadable.java:
##
@@ -54,12 +69,11 @@ public interface ByteBufferPositionedReadable {
* stream supports this interface, otherwise they might get a
* {@link UnsupportedOperationException}.
*
- * 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.
- *
- * 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.
Review 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
> Add more tests for ByteBufferPositionedReadable
>
>
> Key: HDFS-15042
> URL: https://issues.apache.org/jira/browse/HDFS-15042
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: fs, test
>Affects Versions: 3.3.0
>Reporter: Steve Loughran
>Assignee: Steve Loughran
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> There's a few corner cases of ByteBufferPositionedReadable which need to be
> tested, mainly illegal read positions. Add them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17577010#comment-17577010
]
ASF GitHub Bot commented on HDFS-15042:
---
virajjasani commented on code in PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#discussion_r940734766
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ByteBufferPositionedReadable.java:
##
@@ -54,12 +69,11 @@ public interface ByteBufferPositionedReadable {
* stream supports this interface, otherwise they might get a
* {@link UnsupportedOperationException}.
*
- * 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.
- *
- * 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.
Review 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;
}
```
##
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/fs/ByteBufferPositionedReadable.java:
##
@@ -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.
+ *
* Implementers of this interface provide a positioned read API that writes to
a
* {@link ByteBuffer} rather than a {@code byte[]}.
- *
+ *
+ * Thread safety
+ *
+ * These operations doe not change the current offset of a stream as returned
Review Comment:
nit: s/doe/does
> Add more tests for ByteBufferPositionedReadable
>
>
> Key: HDFS-15042
> URL: https://issues.apache.org/jira/browse/HDFS-15042
> Project: Hadoop HDFS
> Issue Type: Improvement
> Components: fs, test
>Affects Versions: 3.3.0
>Reporter: Steve Loughran
>Assignee: Steve Loughran
>Priority: Major
> Labels: pull-request-available
> Time Spent: 1.5h
> Remaining Estimate: 0h
>
> There's a few corner cases of ByteBufferPositionedReadable which need to be
> tested, mainly illegal read positions. Add them
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
-
To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[ https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17576671#comment-17576671 ] ASF GitHub Bot commented on HDFS-15042: --- steveloughran commented on PR #1747: URL: https://github.com/apache/hadoop/pull/1747#issuecomment-1207908058 @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 > Add more tests for ByteBufferPositionedReadable > > > Key: HDFS-15042 > URL: https://issues.apache.org/jira/browse/HDFS-15042 > Project: Hadoop HDFS > Issue Type: Improvement > Components: fs, test >Affects Versions: 3.3.0 >Reporter: Steve Loughran >Assignee: Steve Loughran >Priority: Major > Labels: pull-request-available > Time Spent: 1.5h > Remaining Estimate: 0h > > There's a few corner cases of ByteBufferPositionedReadable which need to be > tested, mainly illegal read positions. Add them -- This message was sent by Atlassian Jira (v8.20.10#820010) - To unsubscribe, e-mail: hdfs-issues-unsubscr...@hadoop.apache.org For additional commands, e-mail: hdfs-issues-h...@hadoop.apache.org
[jira] [Commented] (HDFS-15042) Add more tests for ByteBufferPositionedReadable
[
https://issues.apache.org/jira/browse/HDFS-15042?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17576099#comment-17576099
]
ASF GitHub Bot commented on HDFS-15042:
---
hadoop-yetus commented on PR #1747:
URL: https://github.com/apache/hadoop/pull/1747#issuecomment-1207111762
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|::|--:|:|::|:---:|
| +0 :ok: | reexec | 0m 43s | | Docker mode activated. |
_ Prechecks _ |
| +1 :green_heart: | dupname | 0m 1s | | No case conflicting files
found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available.
|
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain
any @author tags. |
| +1 :green_heart: | test4tests | 0m 0s | | The patch appears to
include 2 new or modified test files. |
_ trunk Compile Tests _ |
| +0 :ok: | mvndep | 15m 9s | | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 25m 32s | | trunk passed |
| +1 :green_heart: | compile | 23m 5s | | trunk passed with JDK
Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | compile | 20m 53s | | trunk passed with JDK
Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | checkstyle | 4m 23s | | trunk passed |
| +1 :green_heart: | mvnsite | 6m 14s | | trunk passed |
| +1 :green_heart: | javadoc | 4m 54s | | trunk passed with JDK
Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 5m 2s | | trunk passed with JDK
Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 10m 18s | | trunk passed |
| +1 :green_heart: | shadedclient | 24m 24s | | branch has no errors
when building and testing our client artifacts. |
_ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 34s | | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 26s | | the patch passed |
| +1 :green_heart: | compile | 22m 22s | | the patch passed with JDK
Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javac | 22m 22s | | the patch passed |
| +1 :green_heart: | compile | 20m 50s | | the patch passed with JDK
Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | javac | 20m 50s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks
issues. |
| +1 :green_heart: | checkstyle | 4m 15s | | root: The patch generated
0 new + 45 unchanged - 5 fixed = 45 total (was 50) |
| +1 :green_heart: | mvnsite | 6m 2s | | the patch passed |
| +1 :green_heart: | javadoc | 4m 55s | | the patch passed with JDK
Private Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1 |
| +1 :green_heart: | javadoc | 5m 4s | | the patch passed with JDK
Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| +1 :green_heart: | spotbugs | 10m 47s | | the patch passed |
| +1 :green_heart: | shadedclient | 24m 39s | | patch has no errors
when building and testing our client artifacts. |
_ Other Tests _ |
| +1 :green_heart: | unit | 18m 53s | | hadoop-common in the patch
passed. |
| +1 :green_heart: | unit | 3m 16s | | hadoop-hdfs-client in the patch
passed. |
| +1 :green_heart: | unit | 236m 38s | | hadoop-hdfs in the patch
passed. |
| +1 :green_heart: | asflicense | 1m 58s | | The patch does not
generate ASF License warnings. |
| | | 505m 42s | | |
| Subsystem | Report/Notes |
|--:|:-|
| Docker | ClientAPI=1.41 ServerAPI=1.41 base:
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-1747/4/artifact/out/Dockerfile
|
| GITHUB PR | https://github.com/apache/hadoop/pull/1747 |
| Optional Tests | dupname asflicense compile javac javadoc mvninstall
mvnsite unit shadedclient spotbugs checkstyle codespell detsecrets |
| uname | Linux dd0b9f467640 4.15.0-112-generic #113-Ubuntu SMP Thu Jul 9
23:41:39 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/bin/hadoop.sh |
| git revision | trunk / ac0bcd8e379f186a5d244284ef4ffcd640c89ed6 |
| Default Java | Private Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Multi-JDK versions | /usr/lib/jvm/java-11-openjdk-amd64:Private
Build-11.0.15+10-Ubuntu-0ubuntu0.20.04.1
/usr/lib/jvm/java-8-openjdk-amd64:Private
Build-1.8.0_312-8u312-b07-0ubuntu1~20.04-b07 |
| Test Results |
https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-1747/4/testReport/ |
| Max. process+thread count | 3489 (vs.
