move retries lower and retry rename ops #82
Conversation
Instead or retrying the whole operation, retry the specific parts that can fail. I believe this also fixes cases where we might end up with a retry loop within a retry loop? I'm not sure.
Retry renames when the error wasn't due to a file not existing. This case can come up on windows when some other process opens the file for reading while we're trying to rename it.
|
@willscott could you check this logic thoroughly. I think I got everything right, but I'm not sure. |
| for i := 0; i < RetryAttempts; i++ { | ||
| err = os.Rename(tmpPath, path) | ||
| // if there's no error, or the source file doesn't exist, abort. | ||
| if err == nil || os.IsNotExist(err) { |
There was a problem hiding this comment.
Not sure if it's worth going down this rabbit hole of understanding what classes of errors are possible here.
- it looks like you'll get an
EEXISTerror in some cases if you try renaming a file to itself. - renaming across disks (https://stackoverflow.com/questions/50740902/move-a-file-to-a-different-drive-with-go) can fail in some cases. I wonder what happens when your tmp dir is on a different disk than your main data cache.
There was a problem hiding this comment.
We're putting the temporary directory inside the datastore directory, so that shouldn't be an issue.
Really, we probably don't even need "is not exist", but we might as well have it.
Yeah. I don't think we should be retrying write operations, but we probably should retry remove operations. |
Co-Authored-By: Will <will.scott@protocol.ai>
|
Ok, I've added retries to the delete ops as well. |
No description provided.