Include content length in the response of Get and GetRange #145
Conversation
ashwanthgoli
force-pushed
the
get-include-size
branch
from
369f1ef to
9b20509
Compare
ashwanthgoli
force-pushed
the
get-include-size
branch
from
a07aac6 to
eb3e5a4
Compare
providers/gcs/gcs.go
Outdated
| return r, err | ||
| } | ||
|
|
||
| return objstore.ObjectSizerReadCloser{ReadCloser: r, Size: r.Attrs.Size}, nil |
There was a problem hiding this comment.
Should the size be set to length here?
There was a problem hiding this comment.
https://github.com/googleapis/google-cloud-go/blob/main/storage/reader.go#L39
this field holds the length according to the source
There was a problem hiding this comment.
This is where that value is set: https://github.com/googleapis/google-cloud-go/blob/main/storage/grpc_client.go#L1126.
According to the code, this is the size of the entire object, even if a range is requested: https://github.com/googleapis/google-cloud-go/blob/main/storage/grpc_client.go#L1109-L1110.
We might want to test this better to be sure we're returning the right value.
There was a problem hiding this comment.
thanks! adding it for GetRange was an after thought, i'll have another look
There was a problem hiding this comment.
Remain() looks like the right value to use for Range. wdyt?
https://github.com/googleapis/google-cloud-go/blob/main/storage/grpc_client.go#L1151
https://github.com/googleapis/google-cloud-go/blob/main/storage/http_client.go#L1434
There was a problem hiding this comment.
Yes, this could be the one that we need. We have e2e tests that run in the CI which could be useful for this: https://github.com/thanos-io/objstore/blob/main/testing.go#L83.
There was a problem hiding this comment.
other providers are returning content length which i am hoping reflects the requested range, I'll try to do a basic sanity check for a couple of providers tomorrow.
if this feels risky, i can limit the change to Get call alone
There was a problem hiding this comment.
👍 i'll add something there :)
There was a problem hiding this comment.
@fpetkovski added the tests, it's looking good. can you take another look when you get a chance? Thanks!
providers/s3/s3.go
Outdated
| return o, err | ||
| } | ||
|
|
||
| return objstore.ObjectSizerReadCloser{ReadCloser: o, Size: stat.Size}, nil |
There was a problem hiding this comment.
references:
size is taken from Content-Length header
https://github.com/minio/minio-go/blob/master/utils.go#L264
https://docs.aws.amazon.com/AmazonS3/latest/API/API_HeadObject.html
HeadObject returns only the metadata for an object. If the Range is satisfiable, only the ContentLength is affected in the response. If the Range is not satisfiable, S3 returns a 416 - Requested Range Not Satisfiable error.
There was a problem hiding this comment.
Isnt this adding an additional request to the GetRange call?
There was a problem hiding this comment.
minio-go saves the objectInfo when we get the object. calling Stat() after a successful GetObject() call won't trigger the HEAD request i think.
this is very likely not going to cause a regression since this code executes only when some asks for Size().
but let me know if you notice any problems :)
ashwanthgoli
force-pushed
the
get-include-size
branch
2 times, most recently
from
445546e to
b7187b0
Compare
|
|
||
| return objstore.ObjectSizerReadCloser{ | ||
| ReadCloser: file, | ||
| Size: file.Length, |
There was a problem hiding this comment.
file.Length() returns the value of Content-Length response header if it is set, else it makes a HEAD call to the store
|
|
||
| type ObjectSizerReadCloser struct { | ||
| io.ReadCloser | ||
| Size func() (int64, error) |
There was a problem hiding this comment.
From what I see, we always return nil as the error, so I wonder why we need to have it as a return argument.
There was a problem hiding this comment.
swift could potentially return err if the HEAD call fails
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
Signed-off-by: Ashwanth Goli <iamashwanth@gmail.com>
ashwanthgoli
force-pushed
the
get-include-size
branch
from
512cd52 to
dfed39a
Compare
|
@fpetkovski can you help merge this change? :) |
This pr adds implements
ObjectSize()method on the return values ofGetandGetRange. This should help readers preallocate buffers based on the content size to reduce number of allocations. They can callTryToGetSizeon the reader to get the size, existing comment already highlights that this is best effort.I did not add it for swift since the Length call could potentially result in another request to the serverChanges
Verification
Updated the acceptance test to check for object size after Get and GetRange calls