facebookarchive / rocks-strata

Other
133 stars 24 forks source link

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

Closed gitgaatachal closed 7 years ago

gitgaatachal commented 7 years ago

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

AGFeldman commented 7 years ago

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: https://github.com/influxdata/influxdb/issues/6235#issuecomment-206231289 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:

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 commented 7 years ago

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 commented 7 years ago

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:

Merging, thanks!