s3: review suggestions #1
Conversation
- relates to S3 driver: Attempt HeadObject on Stat first, fail over to List distribution/distribution#4401
- statList: create returned value inline
- statList, statHead: internalize s3Path, and set Path field
- Stat: use errors.As
- statList: create returned value inline - statList, statHead: internalize s3Path, and set Path field - Stat: use errors.As Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| IsDir: false, | ||
| Size: *resp.ContentLength, | ||
| ModTime: *resp.LastModified, | ||
| }, nil | ||
| } | ||
|
|
||
| func (d *driver) statList(ctx context.Context, s3Path string) (*storagedriver.FileInfoFields, error) { | ||
| func (d *driver) statList(ctx context.Context, path string) (*storagedriver.FileInfoFields, error) { | ||
| s3Path := d.s3Path(path) |
There was a problem hiding this comment.
the reason why this was done in Stat is so the strings.Trimming done by d.s3Path is only ever done once regardless of any potential fall-through from head to list; this PR does it in some cases in both statHead and listHead. I really don't believe there's any need for that.
There was a problem hiding this comment.
But it's still done only once for each of those functions; path is never mutated ahead of time? Only because of that, creating FileInfoFields was not really atomic; the caller had to be aware that Path is not set, so has to be set manually.
There was a problem hiding this comment.
once for each of those functions
That's exactly my point here; we only ever do it once in Stat. I'm trying to explain why that code was written like that and why Corey's suggestion was followed. Again, this code does it once in a successful head case and at worst twice.
| return &storagedriver.FileInfoFields{ | ||
| Path: path, | ||
| IsDir: true, | ||
| }, nil | ||
| } | ||
| fi.IsDir = false | ||
| fi.Size = *resp.Contents[0].Size | ||
| fi.ModTime = *resp.Contents[0].LastModified | ||
| return &fi, nil | ||
| return &storagedriver.FileInfoFields{ | ||
| Path: path, | ||
| Size: *resp.Contents[0].Size, | ||
| ModTime: *resp.Contents[0].LastModified, | ||
| }, nil | ||
| } | ||
| if len(resp.CommonPrefixes) == 1 { | ||
| fi.IsDir = true | ||
| return &fi, nil | ||
| return &storagedriver.FileInfoFields{ | ||
| Path: path, | ||
| IsDir: true, | ||
| }, nil |
There was a problem hiding this comment.
I can't find a reason in my head to justify all of these add extra lines of code. But maybe you'll persuade me.
There was a problem hiding this comment.
Formatting aside (these shorter ones can definitely be a 1-line), it's clear at one glance what's returned; previously setting these fields was sprinkled across multiple locations, and even spread across multiple functions, making it more error-prone, and more difficult to see what values are used.
registry/storage/driver/s3-aws/s3.go
Outdated
| if errors.Is(err, storagedriver.PathNotFoundError{}) { | ||
| return nil, storagedriver.PathNotFoundError{Path: path} | ||
| } | ||
| return nil, parseError(path, err) |
There was a problem hiding this comment.
Actually, looks like this can also be reduces now that statList gets the path passed.
There was a problem hiding this comment.
Looks like this code doesn't work;
errors.Is()only matches if astoragedriver.PathNotFoundError{}is returned without aPathset, butstatListreturnsstoragedriver.PathNotFoundError{Path: s3Path}, so it would never be empty- ☝️ that should probably be
errors.Asto detect the type - This code replaces the error (containing
s3Path) and then changes it to use an error withpathinstead.
There was a problem hiding this comment.
errors.Is() breaking on storagedriver.PathNotFoundError is interesting; I find it kinda funny because originally I was actually returning empty storagedriver.PathNotFoundError but then decided to smack s3Path into it -- that value is actually overridden in Stat call but I admit this is an oversight on my side; we should use errors.As there
There was a problem hiding this comment.
I hate the errors.Is/errors.As. Or at least; they're too easy to get wrong, and nothing will immediately reveal that you did so.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
1f54e08
into
milosgajdos:stat-head-object-first