Skip to content

RFC - Cleansing the 🐉🐉🐉 #139

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.

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

Closed
daviddias opened this issue Jun 28, 2017 · 12 comments
Closed

RFC - Cleansing the 🐉🐉🐉 #139

daviddias opened this issue Jun 28, 2017 · 12 comments
Assignees

Comments

@daviddias
Copy link
Member

daviddias commented Jun 28, 2017

It has become extremely complicated to understand what is happening inside this module, the similar names (**store everything), the fact that the API docs were removed in favor of aegir docs but those are not getting published and how things are being initialized.

The API that we need is only:

const options = {
  path: <path to store the repo in>,
  storageBackends: {
    root: <a interface-datastore compatible class> // default to fs-datastore
    blocks: <a interface-datastore compatible class> // default to fs-datastore
    datastore <a interface-datastore compatible class> // default to level-datastore
  }
}
const repo = new Repo(options)

repo.init(cb) // creates the folder structure
repo.open(cb) // locks the repo 
repo.close(cb) // unlocks the repo

// get and put values at the root of the repo
repo.{get(key), put(key, value}  

// get and put blocks. Knows how to do the sharding and appending .data to the keys
repo.blocks.{get(key), put(key, value}  

// get and put kv. Exposes the full interface-datastore API
repo.datastore.{<full interface-datastore API>}  

The lock is a special case just to be compatible with go. In Node.js should continue to use fs.

What I'm proposing

  • Avoid modules like 'sharding datastore' and just do the sharding from within the blocks file here
  • Refactor the code, improve comments and flow
  • Bring back API docs for the README.
  • Write more tests to ensure that there are no breaking changes in the feature that affect how files get stored and how configuration works.
@daviddias
Copy link
Member Author

@kumavis this will require an update on how the 'blockstore' for parity works. But it should be a straightforward update and a simplification of what already exists.

@pgte
Copy link
Contributor

pgte commented Jun 28, 2017

@diasdavid what about repo.config and repo.version?

@daviddias
Copy link
Member Author

daviddias commented Jun 28, 2017

from IRC, you made a good point that it would be cumbersome to always be encoding/decoding those values. It makes sense to have those convenient methods.

There is 3 'convinience' methods in total:

  • repo.config
  • repo.version
  • repo.apiAddr

These should use repo.{get, put} internally.

@victorb
Copy link
Member

victorb commented Jun 28, 2017

Bring back API docs for the README.

Feels like this should be a endeavour to either finish the API docs migration we were working on, or revert it as you mentioned. Applies to a lot of modules, not just js-ipfs-repo though.

@daviddias
Copy link
Member Author

@victorbjelkholm there is a whole endeavor around improving our build and release tools -- ipfs/aegir#113 -- which we need to get back to. However, this should not block the ipfs-repo improvements from moving forward.

@dignifiedquire
Copy link
Member

Avoid modules like 'sharding datastore' and just do the sharding from within the blocks file here

This seems to be step backwards rather than an improvement. I took special care to make datastores as composable and reusable as possible :/

@dignifiedquire
Copy link
Member

Bring back API docs for the README.

Just run the docs generator for markdown and copy paste if this is so urgent

@dignifiedquire
Copy link
Member

repo.{get(key), put(key, value}

I don't think it's a good idea to allow for arbitrary writes at the root of the repo, these should be very limited

@dignifiedquire
Copy link
Member

You missed repo.exists() which is a very useful and needed method

@daviddias
Copy link
Member Author

make datastores as composable and reusable as possible :/

I understand I value that, flexibility and composability are key. I'm not a particular fan of how things are set up internally, it is extremely confusing right now all the Xdatastore nestness (which can reach 4 levels deep, that is a huge 🐲) with little docs to say how the blocks folder is structured.

Right now, it is a complicated to distinguish between the border of what the blocks folder has to look like to what blockstore needs to do (i.e, blocks need to be sharded and extended with .data for fs due to go interop, but it can be whatever the user wants to plug for their own case vs blockstore is just the API that the rest of IPFS consumes).

Creating a simpler set up at the top level will enable things like #139 to happen much faster :)

@pgte
Copy link
Contributor

pgte commented Jun 29, 2017

@diasdavid do you think we need store.blocks.putMany(blocks, cb)?

@daviddias
Copy link
Member Author

It's useful, see https://github.com/ipfs/js-ipfs-block-service/blob/master/src/index.js#L74-L80

We could have a loop there, but the usefulness of doing putMany through bitswap is that then bitswap can only select to provide on the DHT one of the blocks, avoiding to cause a ton of queries at the same time. putMany here makes it convinient.

daviddias pushed a commit that referenced this issue Jul 4, 2017
* feat: API cleansing - closes #139
@daviddias daviddias removed the status/in-progress In progress label Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants