implement ContentServerDataStore for azure blob storage too #1234
Conversation
…more headers and Range without rangeEnd
|
yeah, as suggested, azure supports all those headers too. please have a look, i gave it a new try. tests with azurite do work.. do you see anything else missing? |
There was a problem hiding this comment.
Thank you very much for the quick updates! The handler is the request headers looks much better yet. Could you also have a look at the failing tests at https://github.com/tus/tusd/actions/runs/12736636713/job/35496766413?pr=1234? Talking about tests, it would be great to have some tests for ServeContent in azurestore as well. For inspiration you can use s3store's tests: https://github.com/tus/tusd/blob/main/pkg/s3store/serve_content_test.go
There was a problem hiding this comment.
Hello there, I just had another look over this PR. Apologies for missing it earlier. The PR seems mostly finished. I think the handling of Range header could be improved a bit and then there is still this comment: https://github.com/tus/tusd/pull/1234/files#r1905384839
Let me know if you are still interested in completing this PR. If not, I can consider pulling it across the finish line.
|
|
||
| if val := r.Header.Get("Range"); val != "" { | ||
| // zero value count indicates from the offset to the resource's end, suffix-length is not required | ||
| input.Range = azblob.HTTPRange{Offset: 0, Count: 0} |
There was a problem hiding this comment.
Does Azure support fetching multiple ranges in a single request? I.e. using Range: <unit>=<range-start>-<range-end>, …, <range-startN>-<range-endN> (from https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Range). S3 doesn't so the s3store doesn't support it. We can do the same here, but I was just wondering.
There was a problem hiding this comment.
Hello, thank you for getting back :)
Response headers are forwarded now.
Regarding Range Header handling, what is missing, give me some hints?
Regarding multiple range in a single request, that is not supported. see also https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob@v1.6.1/internal/exported#HTTPRange
There was a problem hiding this comment.
Regarding multiple range in a single request, that is not supported. see also https://pkg.go.dev/github.com/Azure/azure-sdk-for-go/sdk/storage/azblob@v1.6.1/internal/exported#HTTPRange
Ok, I see. Can you please add a comment mentioning that Azure doesn't support multiple ranges and thus azurestore falls back to fetching the full resource, just like s3store does?
There was a problem hiding this comment.
Response headers are forwarded now.
Great, thanks you!
| input.Range = azblob.HTTPRange{Offset: 0, Count: 0} | ||
| bytesEnd := 0 | ||
| if _, err := fmt.Sscanf(val, "bytes=%d-%d", &input.Range.Offset, &bytesEnd); err != nil { | ||
| if _, err := fmt.Sscanf(val, "bytes=%d-", &input.Range.Offset); err != nil { |
There was a problem hiding this comment.
This doesn't seem to be handling requests where only the last bytes are requested using Range: <unit>=-<suffix-length>. Is that supported by Azure?
There was a problem hiding this comment.
that isn't supported either. how should this be handled? throw an error/map to a different httprange?
It would be nice, if you could pull it across the finish line, I'm willing to do the testing :)
There was a problem hiding this comment.
Ok, I see that the Azure SDK doesn't support this natively: https://github.com/Azure/azure-sdk-for-go/blob/f29a1d52c4e6894a536b0da18ac4399692e02c4c/sdk/storage/azblob/internal/exported/exported.go#L23
However, I think the corresponding AzUpload might have the total size of the object already in its info object. So we could calculate the corresponding offset based on the size as offset = size - suffix-length (and maybe -1 depending on how the bytes are counted, not sure).
I don't want to drag this on too long, but if there is an easy way to support fetching trailing bytes, I think it would be great to have it :)
2 participants
Hi & Happy new year
Implement ContentServerDataStore for azure blob storage too, tests with azurite+azure blob are ok.
Incomplete range requests (Range: bytes=100- ) are not supported, hope that's ok
Regards