Rename stores #136
Rename stores #136
Conversation
There was a problem hiding this comment.
This makes total sense @justinmchase. Thank you!
Do you happen to be working on a S3 backend?
fix: remove the extensions .data that got introduced by #136
| }]) | ||
| const DataStore = this.options.dataStore | ||
| const dataStore = new DataStore(path.join(this.path, dataStoreDirectory), this.options.dataStoreOptions) | ||
| log(dataStore) |
| level: require('memdown'), | ||
| lock: 'memory' | ||
| const repos = [ | ||
| { |
There was a problem hiding this comment.
please don't change code style, this makes it hard to read the diff
| const repos = [{ | ||
| name: 'default', | ||
| opts: undefined, | ||
| init: false |
| opts: { | ||
| fs: require('interface-datastore').MemoryDatastore, | ||
| level: require('memdown'), | ||
| lock: 'memory' |
| before((done) => { | ||
| series([ | ||
| (cb) => { | ||
| if (r.init) { |
There was a problem hiding this comment.
why are you removing this?
There was a problem hiding this comment.
For a bad reason. Basically I thought it was an inconsistency in the way the stores were getting created and a bug in this test but now I see that the tests were failing because I dropped the extension: '' from the store options which was making the fs based tests fail because it couldn't find the files copied in the before step. Adding the extension back and reverting basically this whole file resolves it.
| this._fsOptions = Object.assign({}, options.fsOptions) | ||
| const FsStore = this.options.fs | ||
| this._fsStore = new FsStore(this.path, Object.assign({}, this._fsOptions, { | ||
| extension: '' |
There was a problem hiding this comment.
you can't just remove options that are important
There was a problem hiding this comment.
Yup, that was missed when moving options into default options objects.
| this._fsStore = new FsStore(this.path, Object.assign({}, this._fsOptions, { | ||
| extension: '' | ||
| })) | ||
| this.options = Object.assign({ lock: 'memory', sharding: true }, options || defaultOptions) |
There was a problem hiding this comment.
lock still needs to be fs not memory
There was a problem hiding this comment.
Question about this. The fs lock appears to not use the datastore but to use fs api's directly. Supposing you were going to use a virtual file storage, and not the local file system, such as s3 would you still want a file system lock? It's not that the file system is completely off limits from writing to but the machine can get taken down and back up and the file system is ephemeral with the instance. Should the lock file also be stored on external storage or is it tied to the life cycle of the process?
Is its purpose to prevent other apps from writing the same files and if so shouldn't the lock actually be in the one of the stores?
|
I updated my branch by adding the |
I'm intending to add a new abstraction for s3 file storage (#135). This is simply a straight up refactor to rename the stores based on their purpose rather than the names of the kinds of storage. In other words, instead of
fsandlevelthe stores are renamed toblockanddata.The s3 storage type will be in another repo and will be very similar to the fs datastore but saving into s3.