Make GCS HttpHandler more compliant #126007
Conversation
elasticsearchmachine
added
needs:triage
nicktindall
added
>test
elasticsearchmachine
added
the
Team:Distributed Coordination
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
elasticsearchmachine
removed
the
needs:triage
| exchange.sendResponseHeaders(RestStatus.REQUESTED_RANGE_NOT_SATISFIED.getStatus(), -1); | ||
| return; | ||
| } | ||
| if (range.start() >= blob.contents().length()) { |
There was a problem hiding this comment.
I wonder, should we be checking the range end here? If you ask for bytes 0-10 of an 8-byte blob, we cannot satisfy that range.
There was a problem hiding this comment.
Yes, probably. I'll have a look at what the spec says, currently we just send as much of it as we have.
There was a problem hiding this comment.
I'm also adding ETags, because it looks like that's the mechanism that the BlobReadChannel uses to detect that the file it's streaming has changed under it. We can implement them trivially using generation I think.
There was a problem hiding this comment.
So it looks like
- We don't yet check that the file hasn't changed when resuming a download (I raised ES-11432 to address at some point)
- We do need to allow ranges beyond the end of actual content (streaming uses this to request a chunk). There may be more to the story, but if we start enforcing the ranges all that breaks.
There was a problem hiding this comment.
I confirmed this testing against the actual API
- If the start of the range is past the end of the file, you get a 416
- If the end of the range is past the end of the file, but the start is in it, you get a 206 and as much of the file as can be delivered
|
|
||
| container.deleteBlobsIgnoringIfNotExists(randomPurpose(), Iterators.single(key)); | ||
| } | ||
| } |
There was a problem hiding this comment.
There was a bug when we wrote a multiple of the chunk size in bytes, it sends
POST metadata
PUT chunk1
PUT chunk2
...
PUT no data with content-range header to indicate we're done
We had assumed the finished header was sent with the final chunk.
nicktindall
changed the title
4 participants
This PR includes a couple of bug-fixes and some increased compliance with the actual GCS API
RangeheaderRangeheader is specifiedBlobReadChanneluses this to ensure we fail when the blob is updated between successive chunks being fetched)The 416 on zero-length blobs was one of(?) the causes of #125668