SearchableSnapshotIndexInput should read all bytes #52199
Conversation
tlrx
added
>bug
:Distributed Coordination/Snapshot/Restore
|
Pinging @elastic/es-distributed (:Distributed/Snapshot/Restore) |
|
D'oh, of course. Change looks good, but can we engineer a test that fails because of this bug too? |
|
@DaveCTurner the bug was here since the beginning and passed all reviews. I'm just happy that adding S3 integration tests caught this.
Sure, I should have added one, just doing ten things at a time 😓 I pushed 4de4f81 which randomized how read bytes operations are served. I did not add a specific test but we already have assertions, intensive randomized read tests and we'll soon have integration tests so I think we're good. Unless you think otherwise? |
| offset + optimizedReadSize + directReadSize, | ||
| length - optimizedReadSize - directReadSize); | ||
| if (read == -1) { | ||
| break; |
There was a problem hiding this comment.
This means we hit the end of the file, which I think is unexpected. In tests the assertion below will fail; in production we will proceed even though some of the buffer contains its old contents, which would be bad. I think instead we should throw an EOFException here.
There was a problem hiding this comment.
I think instead we should throw an
EOFExceptionhere.
I pushed e06a94b
| assert read <= length : read + " vs " + length; | ||
| pos += read; | ||
| return read; | ||
| int totalRead = 0; |
There was a problem hiding this comment.
Could we extract a method to do this? The two changes to this file look very similar.
|
@DaveCTurner Thanks for your review. I've applied your feedback, would you mind to have another look? |
| @@ -276,7 +296,7 @@ boolean canContinueSequentialRead(int part, long pos) { | |||
|
|
|||
| int read(byte[] b, int offset, int length) throws IOException { | |||
| assert this.pos < maxPos : "should not try and read from a fully-read stream"; | |||
| int read = inputStream.read(b, offset, length); | |||
| final int read = readFully(inputStream, b, offset, length, () -> {}); | |||
There was a problem hiding this comment.
I'm wondering whether we should throw an EOFException if this hits EOF too. Do we need to behave differently here?
There was a problem hiding this comment.
Oh no I get it now, we might use this to read past the end of a stream that we previously opened. Never mind.
|
Waiting for #52258 to be merged first. |
|
Thanks David |
This pull request fixes a bug in
SearchableSnapshotIndexInputwhich does not use theInputStream.read(byte[], int, int)method correctly.The javadoc indicates that:
Consequently,
SearchableSnapshotIndexInputshould try to read more bytes when a first reading operation has not brought back all the requested bytes.