CASSANALYTICS-147: BufferingInputStream fails to read last chunk

[lukasz-antoniak]

Fixes CASSANALYTICS-147.

[lukasz-antoniak]

I am not sure how effective the change in BufferingInputStream would affect skip() used during BIG index reading. All integration tests pass though, and I think hereby unit test is just a simulation.

[lukasz-antoniak]

According to below JavaDoc, ranges should be considered inclusive.

/**
 * Asynchronously request bytes for the SSTable file component in the range start-end, and pass on to the StreamConsumer when available.
 * The start-end range is inclusive.
 *
 * @param start    the start of the bytes range
 * @param end      the end of the bytes range
 * @param consumer the StreamConsumer to return the bytes to when the request is complete
 */
void request(long start, long end, StreamConsumer consumer);

[yifan-c]

Yes. The range is inclusive. Underlying, it sends an HTTP range request to sidecar to get the range of data.

[arjunashok]

Seems like FileSystemSource.request() has a workaround (lines 94–99) that was compensating for the old off-by-one in BufferingInputStream.requestMore().
Comment in line 97: "Start-end range is inclusive but on the final request end == length so we need to exclude".

With this fix, end is now at most source.size() - 1, and length equals source.size(), so length <= end is always false.
As a result -

1. the code always takes the increment = 1 path, which happens to produce the correct end - start + 1, so no data corruption, but it's confusing dead code.

2. More importantly, close is never true, so the autoClose path in the finally block never triggers.
   For sequential reads via FileSystemSSTable (non-BTI format, where autoClose = true), the RandomAccessFile handle is never closed after the last chunk, which is a file descriptor leak.

[arjunashok]

Minor: We might want to assert on returnedBuffers.size() == 2 to catch regressions where extra or missing requests are issued

[skoppu22]

Is this calculation still correct after this fix? For example, if position is 50, does it mean BufferingInputStream should start reading from the offset 50? If yes, don't we need to set both bytesRead and bytesWritten to 50 in that case? Whereas this code will set both of them to 51. Is this expected?

[lukasz-antoniak]

For example, if position is 50, does it mean BufferingInputStream should start reading from the offset 50?

Correct, and rangeStart = position.
bytesRead and bytesWritten should be 51 in this case. Count of bytes written and read is 1-based.

[lukasz-antoniak]

@skoppu22, I take it back. This was actually a root cause of my issue! I was always considering +1 due to condition:

public boolean isFinished()
{
    return bytesWritten() >= source.size();
}

I got confused with bytesWritten() and assumed it returns position-like index which need +1 adjustment at the beginning. My bad. Of course you and Yifan were right.

[skoppu22]

If both start and end are inclusive, don't we need to modify dataSize = end - start + 1 ? Perhaps we can catch this off by a byte by adding content/length verification below.

[skoppu22]

Existing bug requests 1 byte extra. Looks like after this change, CdcRandomAccessReader's rebuffer requests 2 bytes extra for backward seek case. Assume offset is 0, buffer remaining is 50, then we will request from 0 to 51, which will be 2 bytes extra. Don't we need to fix this code in CdcRandomAccessReader ?

                    // attempting to read bytes previously read
                    // in practice we read the Commit Logs sequentially
                    // but we still need to respect random access reader API, it will just require blocking
                    int requestLen = buffer.remaining() + 1;
                    long end = offset + requestLen;
                    BlockingStreamConsumer streamConsumer = new BlockingStreamConsumer();
                    source.request(offset, end, streamConsumer);

[skoppu22]

Looks like this code was always off by 2, this PR didn't change this.

[lukasz-antoniak]

@skoppu22, can you re-check after latest changes? From what I have tested, there is no extra offset now.

[skoppu22]

Looks good

[lukasz-antoniak]

@arjunashok, great catch about autoClose. Thank you, issue should be solved now.

[skoppu22]

This comment can be removed as you fixed the problem
// Backward seek path has a bug: requests buffer.remaining() + 1 bytes