qubole / rubix

Cache File System optimized for columnar formats and object stores
Apache License 2.0
183 stars 74 forks source link

Use positional read APIs for downloading file from cloud FS #356

Closed raunaqmorarka closed 4 years ago

shubhamtagra commented 4 years ago

With current block-size of 1MB, it is okay to do positional reads over List as two readRequests in the list will atleast be 1MB apart. But if we ever bring down the block size to much smaller value in future, then positional reads might cause slowness.

shubhamtagra commented 4 years ago

RemoteReadRequestChain can also use positional reads for all but first and the last ReadRequest

raunaqmorarka commented 4 years ago

RemoteReadRequestChain can also use positional reads for all but first and the last ReadRequest

Didn't get the part about first and last ReadRequest. When I tried to convert all reads in RemoteReadRequestChain to positioned reads, I got failures in TestCachingInputStream#testChunkCachingAndEviction

raunaqmorarka commented 4 years ago

With current block-size of 1MB, it is okay to do positional reads over List as two readRequests in the list will atleast be 1MB apart. But if we ever bring down the block size to much smaller value in future, then positional reads might cause slowness.

Right, I think it would take very small block size and lots of holes in the read requests list to cause slowdown. Suppose steaming reads do better in such case by reading additional data and discarding it, then we could simply stick to larger block size in Rubix as that will at least cache the extra data downloaded.

shubhamtagra commented 4 years ago

RemoteReadRequestChain can also use positional reads for all but first and the last ReadRequest

Didn't get the part about first and last ReadRequest. When I tried to convert all reads in RemoteReadRequestChain to positioned reads, I got failures in TestCachingInputStream#testChunkCachingAndEviction

First and lost blocks of RemoteRRC can contain prefix/suffix parts too which are created to align the read to block boundary. These parts would be < 1MB hence better to read first block with streaming read so that when we copy the prefix into prefixBuffer we might read ahead and get rest of the data for dataBuffer. Same for last block.

The tests should not fail due to this, this is an optimization and positional reads can still be used

raunaqmorarka commented 4 years ago

RemoteReadRequestChain can also use positional reads for all but first and the last ReadRequest

Didn't get the part about first and last ReadRequest. When I tried to convert all reads in RemoteReadRequestChain to positioned reads, I got failures in TestCachingInputStream#testChunkCachingAndEviction

First and lost blocks of RemoteRRC can contain prefix/suffix parts too which are created to align the read to block boundary. These parts would be < 1MB hence better to read first block with streaming read so that when we copy the prefix into prefixBuffer we might read ahead and get rest of the data for dataBuffer. Same for last block.

The tests should not fail due to this, this is an optimization and positional reads can still be used

Why are we reading the prefix and suffix parts separately in RemoteRRC ? We seem to be making 3 contiguous reads, can we make a single ranged read from readRequest.backendReadStart to readRequest.backendReadEnd here ? Apart from RemoteRRC, rest of the changes are ready.

shubhamtagra commented 4 years ago

RemoteReadRequestChain can also use positional reads for all but first and the last ReadRequest

Didn't get the part about first and last ReadRequest. When I tried to convert all reads in RemoteReadRequestChain to positioned reads, I got failures in TestCachingInputStream#testChunkCachingAndEviction

First and lost blocks of RemoteRRC can contain prefix/suffix parts too which are created to align the read to block boundary. These parts would be < 1MB hence better to read first block with streaming read so that when we copy the prefix into prefixBuffer we might read ahead and get rest of the data for dataBuffer. Same for last block. The tests should not fail due to this, this is an optimization and positional reads can still be used

Why are we reading the prefix and suffix parts separately in RemoteRRC ? We seem to be making 3 contiguous reads, can we make a single ranged read from readRequest.backendReadStart to readRequest.backendReadEnd here ? Apart from RemoteRRC, rest of the changes are ready.

Because the buffers used are different, client buffer is sized as per actual read and cannot accommodate prefix/suffix.