fix: Transform ObjectExistsWithSize into GetAttributes
#14329
Conversation
|
|
- We're not correctly returning the size of the file when there's no error
DylanGuedes
force-pushed
the
return-size-correctly-s3-api
branch
from
82cdd4b to
dcc914e
Compare
DylanGuedes
changed the title
ObjectExistsWithSize s3 implementation not returning size correctlyObjectExistsWithSize into ObjectSize
DylanGuedes
changed the title
ObjectExistsWithSize into ObjectSizeObjectExistsWithSize into GetAttributes
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
To make it consistent across the different implementations
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| return err == nil, err |
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
To make it consistent across the different implementations
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| return err == nil, err |
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil | ||
| } |
There was a problem hiding this comment.
To make it consistent across the different implementations
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| } | |
| return err == nil, err |
| if err != nil { | ||
| return false, err | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
To make it consistent across the different implementations
| if err != nil { | |
| return false, err | |
| } | |
| return true, nil | |
| return err == nil, err |
There was a problem hiding this comment.
That's super fair but I found a problem with the approach I was following so I had to rewrite everything. I should only return an error if the error isn't NotFound. Not all clients support it which is ok but some of them support it so the rewrite was necessary to keep the old behavior.
| return b.objectAttributes(ctx, objectKey, "azure.GetAttributes") | ||
| } | ||
|
|
||
| func (b *BlobStorage) objectAttributes(ctx context.Context, objectKey, source string) (client.ObjectAttributes, error) { |
There was a problem hiding this comment.
nit: IIUC using a different source is just going to create a different metric? Does it make sense to separate calls to this function into separate metrics?
My suggestion would be to just drop the source parameter, if I'm understanding the code correctly
There was a problem hiding this comment.
Yes, the idea is to allow disambiguation between GetAttributes and ObjectExists where it is possible to do it. Loki isn't using GetAttributes so this shouldn't cause any problems but if you're using Loki's API for doing something, having this separation will help your application separating what is leading the object storage requests.
What this PR does / why we need it:
ObjectExistsWithSizeAPI intoGetAttributesto conform with the existing Thanos API.ObjectSizefalse, niltoObjectExistswhen the only error seen is aNotFound. If someone callsObjectExistsit should returnfalse, nilto aNotExisterror, instead offalse, errWhich issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.mdguide (required)featPRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.mdproduction/helm/loki/Chart.yamland updateproduction/helm/loki/CHANGELOG.mdandproduction/helm/loki/README.md. Example PRdeprecated-config.yamlanddeleted-config.yamlfiles respectively in thetools/deprecated-config-checkerdirectory. Example PR