Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename stores #136

Merged
merged 3 commits into from
Jun 4, 2017
Merged

Rename stores #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
402b94f
Updating store names to block and data
justinmchase May 26, 2017
9b36df1
Always init, and init level for browser correctly.
justinmchase Jun 4, 2017
4750348
Removing temp logging
justinmchase Jun 4, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/default-options-browser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@

// Default configuration for a repo in the browser
module.exports = {
fs: require('datastore-level'),
sharding: false,
fsOptions: {
db: require('level-js')
},
level: require('level-js')
blockStore: require('datastore-level'),
blockStoreOptions: { db: require('level-js') },
dataStore: require('datastore-level'),
dataStoreOptions: { db: require('level-js') },
sharding: false
}
6 changes: 3 additions & 3 deletions src/default-options.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

// Default configuration for a repo in node.js
module.exports = {
fs: require('datastore-fs'),
level: require('leveldown'),
sharding: true
blockStore: require('datastore-fs'),
dataStore: require('datastore-level'),
dataStoreOptions: { db: require('leveldown') }
}
73 changes: 34 additions & 39 deletions src/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const MountStore = core.MountDatastore
const ShardingStore = core.ShardingDatastore

const Key = require('interface-datastore').Key
const LevelStore = require('datastore-level')
const waterfall = require('async/waterfall')
const series = require('async/series')
const parallel = require('async/parallel')
Expand All @@ -22,8 +21,8 @@ const blockstore = require('./blockstore')
const log = debug('repo')

const apiFile = new Key('api')
const flatfsDirectory = 'blocks'
const levelDirectory = 'datastore'
const blockStoreDirectory = 'blocks'
const dataStoreDirectory = 'datastore'
const repoVersion = 5

/**
Expand All @@ -34,33 +33,25 @@ class IpfsRepo {
/**
* @param {string} repoPath - path where the repo is stored
* @param {object} options - Configuration
* @param {Datastore} options.fs
* @param {Leveldown} options.level
* @param {object} [options.fsOptions={}]
* @param {datastore} options.blockStore
* @param {datastore} options.dataStore
* @param {object} [options.blockStoreOptions={}]
* @param {object} [options.dataStoreOptions={}]
* @param {bool} [options.sharding=true] - Enable sharding (flatfs on disk), not needed in the browser.
* @param {string} [options.lock='fs'] - Either `fs` or `memory`.
*/
constructor (repoPath, options) {
assert.equal(typeof repoPath, 'string', 'missing repoPath')

if (options == null) {
options = require('./default-options')
}

const defaultOptions = require('./default-options')
this.closed = true
this.path = repoPath
this.options = Object.assign({
sharding: true,
lock: 'fs'
}, options)
this._fsOptions = Object.assign({}, options.fsOptions)
const FsStore = this.options.fs
this._fsStore = new FsStore(this.path, Object.assign({}, this._fsOptions, {
extension: ''
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't just remove options that are important

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fixed this in #138

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that was missed when moving options into default options objects.

}))
this.options = Object.assign({ lock: 'memory', sharding: true }, options || defaultOptions)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lock still needs to be fs not memory

Copy link
Contributor Author

@justinmchase justinmchase Jun 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const BlockStore = this.options.blockStore
this._blockStore = new BlockStore(this.path, this.options.blockStoreOptions)

this.version = version(this._fsStore)
this.config = config(this._fsStore)
this.version = version(this._blockStore)
this.config = config(this._blockStore)

if (this.options.lock === 'memory') {
this._locker = require('./lock-memory')
Expand All @@ -82,7 +73,7 @@ class IpfsRepo {
log('initializing at: %s', this.path)

series([
(cb) => this._fsStore.open((err) => {
(cb) => this._blockStore.open((err) => {
if (err && err.message === 'Already open') {
return cb()
}
Expand All @@ -108,7 +99,7 @@ class IpfsRepo {

// check if the repo is already initialized
waterfall([
(cb) => this._fsStore.open((err) => {
(cb) => this._blockStore.open((err) => {
if (err && err.message === 'Already open') {
return cb()
}
Expand All @@ -121,8 +112,8 @@ class IpfsRepo {
this.lockfile = lck

log('creating flatfs')
const FsStore = this.options.fs
const s = new FsStore(path.join(this.path, flatfsDirectory), this._fsOptions)
const BlockStore = this.options.blockStore
const s = new BlockStore(path.join(this.path, blockStoreDirectory), this.options.blockStoreOptions)

if (this.options.sharding) {
const shard = new core.shard.NextToLast(2)
Expand All @@ -131,17 +122,21 @@ class IpfsRepo {
cb(null, s)
}
},
(flatfs, cb) => {
(blockStore, cb) => {
log('Flatfs store opened')
this.store = new MountStore([{
prefix: new Key(flatfsDirectory),
datastore: flatfs
}, {
prefix: new Key('/'),
datastore: new LevelStore(path.join(this.path, levelDirectory), {
db: this.options.level
})
}])
const DataStore = this.options.dataStore
const dataStore = new DataStore(path.join(this.path, dataStoreDirectory), this.options.dataStoreOptions)
log(dataStore)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

this.store = new MountStore([
{
prefix: new Key(blockStoreDirectory),
datastore: blockStore
},
{
prefix: new Key('/'),
datastore: dataStore
}
])

this.blockstore = blockstore(this)
this.closed = false
Expand Down Expand Up @@ -197,14 +192,14 @@ class IpfsRepo {

log('closing at: %s', this.path)
series([
(cb) => this._fsStore.delete(apiFile, (err) => {
(cb) => this._blockStore.delete(apiFile, (err) => {
if (err && err.message.startsWith('ENOENT')) {
return cb()
}
cb(err)
}),
(cb) => this.store.close(cb),
(cb) => this._fsStore.close(cb),
(cb) => this._blockStore.close(cb),
(cb) => {
log('unlocking')
this.closed = true
Expand Down Expand Up @@ -235,7 +230,7 @@ class IpfsRepo {
* @returns {void}
*/
setApiAddress (addr, callback) {
this._fsStore.put(apiFile, Buffer.from(addr.toString()), callback)
this._blockStore.put(apiFile, Buffer.from(addr.toString()), callback)
}

/**
Expand All @@ -245,7 +240,7 @@ class IpfsRepo {
* @returns {void}
*/
apiAddress (callback) {
this._fsStore.get(apiFile, (err, rawAddr) => {
this._blockStore.get(apiFile, (err, rawAddr) => {
if (err) {
return callback(err)
}
Expand Down
37 changes: 15 additions & 22 deletions test/node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,35 +13,28 @@ const expect = chai.expect
const IPFSRepo = require('../src')

describe('IPFS Repo Tests on on Node.js', () => {
const repos = [{
name: 'default',
opts: undefined,
init: false
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this removed?

}, {
name: 'memory',
opts: {
fs: require('interface-datastore').MemoryDatastore,
level: require('memdown'),
lock: 'memory'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why was this removed?

const repos = [
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't change code style, this makes it hard to read the diff

name: 'default',
opts: undefined
},
init: true
}]
{
name: 'memory',
opts: {
blockStore: require('interface-datastore').MemoryDatastore,
dataStore: require('interface-datastore').MemoryDatastore
}
}
]
repos.forEach((r) => describe(r.name, () => {
const testRepoPath = path.join(__dirname, 'test-repo')
const date = Date.now().toString()
const repoPath = testRepoPath + '-for-' + date

const repo = new IPFSRepo(repoPath, r.opts)

before((done) => {
series([
(cb) => {
if (r.init) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are you removing this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

repo.init({}, cb)
} else {
ncp(testRepoPath, repoPath, cb)
}
},
(cb) => ncp(testRepoPath, repoPath, cb),
(cb) => repo.init({}, cb),
(cb) => repo.open(cb)
], done)
})
Expand Down Expand Up @@ -73,7 +66,7 @@ describe('IPFS Repo Tests on on Node.js', () => {
require('./repo-test')(repo)
require('./blockstore-test')(repo)
require('./datastore-test')(repo)
if (!r.init) {
if (r.name === 'default') {
require('./interop-test')(repo)
}
}))
Expand Down