Skip to content
This repository has been archived by the owner on Jun 12, 2018. It is now read-only.

Added DriverFactory implementation for local storage and updated comm… #28

Merged
merged 1 commit into from
Oct 8, 2016

Conversation

gitgaatachal
Copy link
Contributor

Added DriverFactory implementation for local storage and updated command line to support local storage

@AGFeldman
Copy link
Contributor

AGFeldman commented Oct 5, 2016

At first glance, looks really good! Hopefully I can review it more carefully within a week.

I think the test fails because MockStorage.PutReader is not thread-safe (https://github.com/facebookgo/rocks-strata/blob/6a5e7852fd8d9005ef0af4ae92782d5ca66df831/strata/mock.go#L171). This isn't hugely concerning, since it is only used for testing other features. FYI: influxdata/influxdb#6235 (comment) suggests that the issue might stay hidden for Go versions <1.6.

Before merging, we/I should review that LStorage is solid, since it too has only been used for testing. TODO:

  • Check LStorage thread-safety
  • Check what happens if a write is interrupted. We already use os.Rename() in LStorage.PutReader to make writes more atomic.

Note that LStorage thread-safety makes some basic assumptions about the filesystem. Like, it assumes that you can concurrently open different files and write to different files.

Thanks!

@gitgaatachal
Copy link
Contributor Author

I have verified commands backup, show backup & restore on ext4 filesystem and everything seems to be working.

Having said that, we will have to do some deep testing on different filesystems too

@AGFeldman
Copy link
Contributor

Looks good to me. Reading LStorage, we don't need much from the filesystem: Atomic rename, atomic delete, and the ability to concurrently create / open / write to / rename / delete different files.

More details:

  • LStorage uses atomic rename so that files aren't left with bad data if a write is interrupted.
  • We don't make concurrent modifications to the same file, unless the user runs multiple write-capable strata commands at once. Which they shouldn't do, but unfortunately the code doesn't prevent it. From the readme: "for a given replica ID, at most one operation that is capable of writing to storage (backup, delete, gc) should be running at any one time." So, the filesystem doesn't need to support concurrent modification of the same file.

Merging, thanks!

@AGFeldman AGFeldman merged commit fc38ec0 into facebookarchive:master Oct 8, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants