Awesome datastore #797

dignifiedquire · 2017-03-17T22:42:24Z

Closes #787
Closes #229

refactored cli to use a context ipfs daemon to ensure cleanup
update to new datastore dependencies
fix timeout issues and add two new events (init and ready) to handle the startup of a daemon more granularly (needs discussion probably)
upgrade to yargs@7

dignifiedquire · 2017-03-20T10:27:59Z

I am afraid this is not ready yet. I've discovered that the new creation and startup api is the source of quite some race conditions, which are becoming more visible with this PR, as the locking of the repo and open and close are more enforced.

We need to think through this very carefully as the options init and start combined with methods for doing those actions are nice for the user but create quite the tricky state management in the background. If we decide to keep the api we need to integrate a proper finite state machine to ensure all transitions are valid and very explicit descriptions on how things work.

Tldr setTimeout usually means you have a race condition you are hiding..

daviddias · 2017-03-20T14:03:04Z

src/cli/bin.js

-  .strict()
-  .completion()
-  .argv
+utils.getIPFS((err, ipfs, cleanup) => {


init and daemon don't use an ipfs instance to run, how does this cover those two cases?

I will add an exception to those, I didn't realise that until later

daviddias · 2017-03-20T14:05:26Z

Note: We can add a init event to the set up, it should solve all of the issues you are seeing as the only issue was when a repo is init'ed by default and gets started 'later', this later is the source of trouble because there is no way to monitor when the init has finished.

The finite state machine approach is also interesting, probably even better to keep the simplicity. Wanna take a stab at it?

daviddias · 2017-03-20T14:56:25Z

See: https://github.com/ipfs/js-ipfs/compare/fix/race?expand=1

daviddias · 2017-03-20T16:04:44Z

Saw now that you added more events.

init is part of master now
ready why the need for this one, exactly?

dignifiedquire · 2017-03-20T16:08:22Z

ready why the need for this one, exactly?

to detect the finishing things like new IPFS({init: false, start: false} which are alway async now, due to the need to detect and open the repo

daviddias · 2017-03-20T16:50:39Z

@dignifiedquire in that case, there are no actions happening asynchronously.

dignifiedquire · 2017-03-20T16:55:01Z

@dignifiedquire in that case, there are no actions happening asynchronously.

but they are now, because repo checks are async

daviddias

Lot's of cool stuff here, 👍 for the state machine approach!

Still needs some changes though.

daviddias · 2017-03-22T15:06:07Z

gulpfile.js

-    start: false,
-    EXPERIMENTAL: {
-      pubsub: true
-    },


why disabling PubSub?

It's not disabled, it is enabled in the httpserver as far as I understand

got it, true. Although it should be really just a config passed. Like @pgte is doing here #805

daviddias · 2017-03-22T15:07:36Z

src/cli/bin.js

+// Need to skip to avoid locking the daemon
+if (args[0] === 'daemon' || args[0] === 'init') {
+  return cli.help().strict(false).completion().parse(args)
+}


Make this more clear:

// commands that don't require a daemon/instance
// commands that require a daemon/instance

Also, use same indentation in both

daviddias · 2017-03-22T15:13:00Z

src/core/components/block.js

+          }
+          cb(null, block)
+        })
+      ], callback)


This has changed the API, block needs to be able to:

» jsipfs block put --help jsipfs block put [block] Options: --help Show help [boolean] --format, -f cid format for blocks to be created with. [default: "dag-pb"] --mhtype multihash hash function [default: "sha2-256"] --mhlen multihash hash length

Before we passed that through CID, if you remove the CID from equation and always calculate the multihash of sha256, then you can't block put eth data anymore.

I don't understand your concern, if you want to specify your own things you pass a regular ipfs block here which already defines the cid it wants, this way things are passed through as expected

the hashing is just a fallback if only a buffer is passed in.

Hm.. I see, you now expect everything in the block.

Can we also have the options object, we will need to support the hashAlg and format. But that can be a separate PR

daviddias · 2017-03-22T15:15:25Z

src/core/components/libp2p.js

-          bootstrap: config.Bootstrap
+          mdns: get(config, 'Discovery.MDNS.Enabled'),
+          webRTCStar: get(config, 'Discovery.webRTCStar.Enabled'),
+          bootstrap: get(config, 'Bootstrap')


Requiring a dependency to pick one property of an object?

This is so that undefined chains don't throw. This code explodes if you try to read a config from go-ipfs as it doesn't have a webRTCStar field in discovery.

daviddias · 2017-03-22T15:15:46Z

src/core/components/pre-start.js

@@ -31,7 +30,7 @@ module.exports = function preStart (self) {

          if (!mafmt.IPFS.matches(ma)) {
            ma = ma.encapsulate('/ipfs/' +
-              self._peerInfo.id.toB58String())
+                                self._peerInfo.id.toB58String())


strange identation

very strange

daviddias · 2017-03-22T15:19:58Z

src/core/status.js

+  running: 'running',
+  stopping: 'stopping',
+  stopped: 'stopped'
+}


Right now, in order to follow the sequence of booting a node, we need to open 4+ files.

Let's put this into the state.js.

Rechecked, this is just used to populate a var this._status once and that const is not used anymore. ???

this is the rest from some experimentation, going to delete it

daviddias · 2017-03-22T15:21:45Z

src/core/utils.js

-    cb()
-  })
-}
-


Where is this now? Breaking API change? If so, update README

I didn't know this was in the readme, this is not needed anymore internally that's why I removed it

Can't find it in the readme, are you sure this was documented somewhere?

Ah, not on the readme, people are using it though - #776

Wanna finish that PR and merge it here?

lets get this merged and then I can do that PR, I don't want to put even more things into here than there are already

daviddias · 2017-03-22T15:22:29Z

src/http-api/index.js

+          })
+        } catch (err) {
+          return cb(err)
+        }


What is throwing? Errors should come on the error event only.

this can throw if someone screwed up in the implementation, resulting in silent fail of the tests

ok, let's add that comment, I want to avoid giving the idea that errors should be 'thrown' for contributors coming into the codebase.

daviddias · 2017-03-22T15:23:34Z

test/go-ipfs-repo/datastore/LOG

@@ -0,0 +1,2 @@
+=============== Mar 18, 2017 (CET) ===============
+00:21:30.447664 log@legend F·NumFile S·FileSize N·Entry C·BadEntry B·BadBlock Ke·KeyError D·DroppedEntry L·Level Q·SeqNum T·TimeElapsed


.gitignore this

daviddias · 2017-03-22T15:23:47Z

test/go-ipfs-repo/version

@@ -0,0 +1 @@
+1


is it really version 1?

hmm no not sure why that says version 1

on disk it says 5 for me

fixed, there were two

daviddias · 2017-03-22T16:28:18Z

Seems that last thing is to rebase master onto this branch :)

daviddias · 2017-03-22T18:45:16Z

I need to leave now for up to 3 hours, will come back online later.

@dignifiedquire, let me know if you can check this.

dignifiedquire · 2017-03-22T23:16:57Z

last missing piece ipfs/js-ipfs-repo#128 (hopefully)

dignifiedquire added the status/in-progress In progress label Mar 17, 2017

dignifiedquire changed the title ~~[WIP] Awesome datastore~~ Awesome datastore Mar 19, 2017

dignifiedquire requested a review from daviddias March 19, 2017 21:35

dignifiedquire mentioned this pull request Mar 20, 2017

🚀 0.23 Release 🌟 #795

Closed

22 tasks

daviddias reviewed Mar 20, 2017

View reviewed changes

dignifiedquire mentioned this pull request Mar 20, 2017

refactor: use new block api ipld/js-ipld-dag-cbor#47

Merged

dignifiedquire force-pushed the feat/datastore branch from 686099d to 6fe86d5 Compare March 20, 2017 17:07

daviddias suggested changes Mar 22, 2017

View reviewed changes

daviddias force-pushed the feat/datastore branch 3 times, most recently from f486cac to d59dc43 Compare March 22, 2017 16:52

daviddias approved these changes Mar 22, 2017

View reviewed changes

daviddias force-pushed the feat/datastore branch from 3146ed4 to b30fc99 Compare March 22, 2017 18:43

daviddias force-pushed the feat/datastore branch from 09bc36c to 68d92b6 Compare March 23, 2017 00:37

feat: datastore, ipfs-block and all the deps that were updated

68d92b6

daviddias merged commit 706ada2 into master Mar 23, 2017

daviddias deleted the feat/datastore branch March 23, 2017 00:45

daviddias removed the status/in-progress In progress label Mar 23, 2017

		@@ -0,0 +1,2 @@
		=============== Mar 18, 2017 (CET) ===============
		00:21:30.447664 log@legend F·NumFile S·FileSize N·Entry C·BadEntry B·BadBlock Ke·KeyError D·DroppedEntry L·Level Q·SeqNum T·TimeElapsed

Awesome datastore #797

Awesome datastore #797

Conversation

dignifiedquire commented Mar 17, 2017 • edited Loading

dignifiedquire commented Mar 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Mar 20, 2017

daviddias commented Mar 20, 2017

daviddias commented Mar 20, 2017

dignifiedquire commented Mar 20, 2017

daviddias commented Mar 20, 2017

dignifiedquire commented Mar 20, 2017

daviddias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias Mar 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daviddias commented Mar 22, 2017 • edited Loading

daviddias commented Mar 22, 2017

dignifiedquire commented Mar 22, 2017

dignifiedquire commented Mar 17, 2017 •

edited

Loading

daviddias Mar 22, 2017 •

edited

Loading

daviddias commented Mar 22, 2017 •

edited

Loading