Add atomic writes to local backend #528
Conversation
Signed-off-by: Mac Chaffee <[email protected]>
Signed-off-by: Mac Chaffee <[email protected]>
| @@ -35,7 +36,20 @@ func NewLocalFilesystemBackend(rootDirectory string) *LocalFilesystemBackend { | |||
| if err != nil { | |||
| panic(err) | |||
| } | |||
| b := &LocalFilesystemBackend{RootDirectory: absPath} | |||
| // Create a temporary folder for partially-completed writes (if not present) | |||
| tempPath := filepath.Join(absPath, ".tmp") | |||
There was a problem hiding this comment.
this temp directory creation might be better suited in the PutObject function (like we do with the RootDirectory). This way the temp directory will always be recreated if it doesn't exist during a put, so if that directory was deleted while chartmuseum is running, uploads will continue to work without restarting chartmuseum.
There was a problem hiding this comment.
That would require an extra stat per invocation of PutObject. Do we really want that performance overhead?
There was a problem hiding this comment.
that is definitely a consideration, but it seems like we should be treating the root dir and this new dir similarly. i.e. either both are created in NewLocalFilesystemBackend or both are created in PutObject. @jdolitsky @scbizu do you have any thoughts here?
There was a problem hiding this comment.
For @cbuto 's consideration , how about do anther os.MkdirAll() and ignore the dir already exist error in PutObject , I agree with cbuto's opinion and it is maybe reasonable even we do some overhead tradeoff here.
Fixes #527
Also I noticed the tests were writing to
../../.testwhich is outside the repo's directory when you runmake test, so I moved that path back to./.test.