HADOOP-17527. ABFS: Fix boundary conditions in InputStream seek and skip #2698
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
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
sumangala-patki
changed the title
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
TEST RESULTS HNS Account Location: East US 2 |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
| @@ -402,6 +399,18 @@ public void testSkipAndAvailableAndPosition() throws Exception { | |||
| inputStream.getPos()); | |||
| assertEquals(testFileLength - inputStream.getPos(), | |||
| inputStream.available()); | |||
|
|
|||
| skipped = inputStream.skip(testFileLength + 1); //goes to last byte | |||
| assertEquals(1, inputStream.available()); | |||
There was a problem hiding this comment.
can you add an error message to all these asserts?
| @@ -583,8 +583,8 @@ public synchronized long skip(long n) throws IOException { | |||
| newPos = 0; | |||
| n = newPos - currentPos; | |||
| } | |||
| if (newPos > contentLength) { | |||
| newPos = contentLength; | |||
| if (newPos >= contentLength) { | |||
There was a problem hiding this comment.
what does skip(0) on a 0-byte file do? I don't think we've ever looked at that?
There was a problem hiding this comment.
skip(0) should seek to 0, corrected and added test
sumangala-patki
changed the title
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
TEST RESULTS HNS Account Location: East US 2 |
|
TEST RESULTS HNS Account Location: East US 2 |
| @@ -100,28 +100,31 @@ public void testSeekStatistics() throws IOException { | |||
| AbfsOutputStream out = null; | |||
| AbfsInputStream in = null; | |||
|
|
|||
| int readBufferSize = getConfiguration().getReadBufferSize(); | |||
| byte[] buf = new byte[readBufferSize + 1]; | |||
There was a problem hiding this comment.
To allow seek to position readBufferSize
Example: readBufferSize = 4, then buf size = 5 => 5 bytes are written to file and valid indices in file are [0 1 2 3 4]
Read call will read readBufferSize bytes, i.e., 4 bytes (up to and incl position 3 in file) so fcursor is now at position 4.
To be counted as a forward seek, any subsequent seek has to be to a position >= current fcursor (4). Hence, the file should have at least 4 + 1 = 5 bytes for seek(4) to be allowed
| in.getPos()); | ||
| in.seek(0); | ||
| assertEquals("Seek to 0 should succeed", 0, in.getPos()); | ||
| in.skip(0); |
There was a problem hiding this comment.
skip returns a long ==> u should assert it is the correct value
There was a problem hiding this comment.
added assertion
bilaharith
reviewed
Feb 23, 2021
| elapsedTimeMs < MAX_ELAPSEDTIMEMS); | ||
|
|
||
| //test negative skip from last valid position | ||
| skipped = inputStream.skip(-testFileLength+1); |
snvijaya
approved these changes
Feb 24, 2021
| assertEquals(1, inputStream.available()); | ||
| bytesRead = inputStream.read(buffer); | ||
| assertEquals(1, bytesRead); | ||
| assertEquals(testFileLength, inputStream.getPos()); |
There was a problem hiding this comment.
getPos() is a public API of InputStream. Does this mean that it can return an invalid position value ?
There was a problem hiding this comment.
getPos() will return contentlength post a read to (incl) EOF; however, seek/skip to invalid position is not supported, so getPos after any of these ops will return valid position
| @@ -100,28 +100,31 @@ public void testSeekStatistics() throws IOException { | |||
| AbfsOutputStream out = null; | |||
| AbfsInputStream in = null; | |||
|
|
|||
| int readBufferSize = getConfiguration().getReadBufferSize(); | |||
There was a problem hiding this comment.
Maybe we can reduce the diff on the test by reducing the bufferSize on the filesystem instance so that rest of the asserts remain valid.
There was a problem hiding this comment.
modified to use original buffer and reduced readBufferSize
bilaharith
approved these changes
Feb 25, 2021
Ojasvi-Jalal
reviewed
Mar 2, 2021
| @@ -542,7 +542,7 @@ public synchronized void seek(long n) throws IOException { | |||
| if (n < 0) { | |||
| throw new EOFException(FSExceptionMessages.NEGATIVE_SEEK); | |||
| } | |||
| if (n > contentLength) { | |||
| if (n > 0 && n >= contentLength) { | |||
| throw new EOFException(FSExceptionMessages.CANNOT_SEEK_PAST_EOF); | |||
There was a problem hiding this comment.
what about when n == 0? also, isn't n>0 unnecessary as it should be implied if the if condition at line 542 is false?
There was a problem hiding this comment.
seek(n=0) is allowed; n>0 has to be specified so as to avoid throwing exception on seek(0) in a 0-byte file
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.
Modify AbfsInputStream seek method to throw EOF exception on seek to contentLength for a non-empty file