Refactor file cache handling and improve S3 rename functionality#847
Refactor file cache handling and improve S3 rename functionality#847jfantinhardesty wants to merge 8 commits into
Conversation
jfantinhardesty
requested review from
Dabnsky,
Ferelith-maker,
brayan-trejo and
foodprocessor
as code owners
foodprocessor
left a comment
foodprocessor
left a comment
There was a problem hiding this comment.
I think this PR is primarily aimed at reducing API call inflation on negative stat calls, which the OS spams us with. I agree with that motivation, but instead of trying to avoid checking for directories by filtering out extensions, maybe we should go the other way and provide a different user flag: require-dir-markers. Then we could stop handling ambiguous cases using expensive and dangerous logic, and instead give the problem back to the user: if you used directory markers from the start, here's a way to guarantee we won't waste time listing prefixes on negative stat calls.
If we did that, it would be nice to include a utility to go through a bucket and add any missing markers.
This is just the vibe I got when reviewing this... that we are bending over backwards for a corner case which is really hurting us, and my feeling is we should relegate the corner case to a corner config. I'll admit, my suggestion doesn't quite do that, but it's a start. This is the kind of thing that almost begs for a 3.0, even though it's not a big enough deal to merit that.
| options.Handle.Path, | ||
| err, | ||
| ) | ||
| return err |
There was a problem hiding this comment.
Shouldn't this be EBADF?
| dirName string, | ||
| explicitDirLookup bool, | ||
| skipDirProbeOnFileExt bool, | ||
| ) bool { |
There was a problem hiding this comment.
Since skipDirProbeOnFileExt always gets cl.Config.skipDirProbeOnFileExt, and that config option's only purpose is to modify this functionality, it feels like this helper should just become a receiver function. Then it can go back to having two parameters.
| if len(ext) < 2 || len(ext) > 5 { | ||
| return false | ||
| } |
There was a problem hiding this comment.
I like the simplicity of this approach, but I think it could be made a bit safer by using more strict checks:
- require there to be something before the dot
- avoid false positives for hidden folders on Linux (e.g.
.git)
- avoid false positives for hidden folders on Linux (e.g.
- require 2-4 characters after the dot
- avoid false positives for directories like
init.don Linux
- avoid false positives for directories like
- returning false for the
.appextension- Copilot tells me these are macOS/app bundles, which are just regular folders under the hood
There was a problem hiding this comment.
To get to the punchline - I'm worried that this new approach prohibits directories with extensions. So, we should be very cautious in doing that, and maybe we should explicitly tell users that this setting hides directories with extensions as non-existent.
I just get nervous any time a corner case looks like data loss. And this one does.
| if (r < '0' || r > '9') && (r < 'A' || r > 'Z') && (r < 'a' || r > 'z') { | ||
| return false | ||
| } | ||
| } |
There was a problem hiding this comment.
Copilot warned me of numeric-only suffixes on directory names (e.g. archive.001/). Should we split the numeric and alpha into two cases and add a flag to check if we saw a letter?
I'm just worried that this heuristic will run into a lot of bad cases when left on by default.
| if cl.Config.enableDirMarker && shouldProbeDirMarker( | ||
| dirName, | ||
| explicitDirLookup, | ||
| cl.Config.skipDirProbeOnFileExt, | ||
| ) { |
There was a problem hiding this comment.
Since we're already in getDirectoryAttr, it seems like we should allow this marker call without checking shouldProbeDirMarker. Especially since headObject is usually faster than List, and definitely cheaper in terms of API cost (on AWS).
| objects, _, listErr := cl.List(ctx, dirName, nil, 1) | ||
|
|
||
| // Otherwise, the cloud does not support directory markers, or there is no | ||
| // marker, so look for an object in the directory. | ||
| if listErr != nil { | ||
| log.Err("Client::getDirectoryAttr : List(%s) failed. Here's why: %v", dirName, listErr) | ||
| return nil, listErr | ||
| } | ||
| if len(objects) > 0 { | ||
| // create and return an objAttr for the directory | ||
| attr := internal.CreateObjAttrDir(dirName) | ||
| return attr, nil | ||
| } | ||
|
|
||
| // Only check for explicit empty directory markers when needed. | ||
| // For file-like names, this saves one extra HeadObject | ||
| // call on miss-heavy paths that are not directories. | ||
| if cl.Config.enableDirMarker && shouldProbeDirMarker( |
There was a problem hiding this comment.
Since headObject is cheaper (especially in terms of AWS API cost), I think we could flip this around and call headObject first, if markers are enabled. I understand that when markers are enabled but no marker is present, that we'll end up calling List as well anyway, but maybe we can treat that as a corner case, and not give first class status to users who switch dir markers on mid-stream.
2 participants
What type of Pull Request is this? (check all applicable)
Describe your changes in brief
Refactored cache space checks into a new
checkCacheSpacemethod infile_cache.go, replacing repetitive code. Removed fallback reopening logic for invalid handles in Truncate. Added a config flag for S3 that defaults to true to skip directory probing for paths ending in file-like extensions, improving performance on miss-heavy paths and refactored function to check for all file-like extensions rather than a few set extensions. Added a check in S3 Storage Rename File to prevent renaming a file to a directory.Checklist
Related Issues