Add snapshot feature to the disk persistence layer #49
Conversation
Implemented a new snapshot method in the disk persistence layer. It allows to store an unique timestamped snapshot file of given data in a dedicated `snapshot` directory.
| @@ -30,13 +33,27 @@ func NewDiskHandle(path string) (Handle, error) { | |||
| return nil, err | |||
| } | |||
|
|
|||
| err = createDir(path, snapshotDir) | |||
There was a problem hiding this comment.
We should probably rename createDir to ensureDirectoryExists at some point (not in this PR though).
There was a problem hiding this comment.
Addressed in the follow-up PR: #50
| @@ -65,6 +82,53 @@ func (ds *diskPersistence) Save(data []byte, dirName, fileName string) error { | |||
| return write(fmt.Sprintf("%s/%s/%s", dirPath, dirName, fileName), data) | |||
| } | |||
|
|
|||
| func (ds *diskPersistence) Snapshot(data []byte, dirName, fileName string) error { | |||
| ds.snapshotMutex.Lock() | |||
| defer ds.snapshotMutex.Unlock() | |||
There was a problem hiding this comment.
It feels like this mutex is primarily meant to protect accidental file overwrites. If that's the case, what do you think about moving the lock + defer to right before the file existence is checked? Otherwise we're capturing more in the mutex than what we're intending to protect.
There was a problem hiding this comment.
Addressed in the follow-up PR: #50
| filePath := fmt.Sprintf("%s/%s/%s", dirPath, dirName, fileName+snapshotSuffix) | ||
|
|
||
| // very unlikely but better fail than overwrite an existing file | ||
| if canWrite := isNotExist(filePath); !canWrite { |
There was a problem hiding this comment.
This if statement feels like it's giving us more code just to invert a condition that can already be articulated. Why not if fileAlreadyExists(filePath) { ...?
There was a problem hiding this comment.
Addressed in the follow-up PR: #50
I've got rid of the canWrite variable and renamed the isNotExist method a bit. But, I think we should stick to the current condition format because we can save the snapshot only if we have the oserror.ErrNotExist error returned from the check. This is easier to check than checking file existence because in case of an existing file the os.Stat method can return not just a nil-error but also several other errors types, for example oserror.ErrPermission.
There was a problem hiding this comment.
Heh, I remember it took me a while to understand how os.Stat works when I was first reviewing this code.
| bytesToTest := []byte{115, 111, 109, 101, 10} | ||
|
|
||
| counter := 0 | ||
| diskHandle.(*diskPersistence).snapshotSuffixGenerator = func() string { |
There was a problem hiding this comment.
I don't love how we've made the suffix generator mutable here, though I see the use for testing. Wish we had a better way to do this, especially the collision test later on.
There was a problem hiding this comment.
Yeah, agree. But a kind of compromise was needed here.
| @@ -85,13 +85,22 @@ func TestSaveReadAndDecryptData(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestEncryptedSnapshot(t *testing.T) { | |||
|
|
|||
There was a problem hiding this comment.
🤦. Removed in the follow-up PR: #50
Implemented a new
Snapshotmethod in the disk persistence layer. It allows storing a unique timestamped snapshotfile of given data in a dedicated
snapshotdirectory.