-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Conversation
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 Tldr |
src/cli/bin.js
Outdated
.strict() | ||
.completion() | ||
.argv | ||
utils.getIPFS((err, ipfs, cleanup) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
init and daemon don't use an ipfs
instance to run, how does this cover those two cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add an exception to those, I didn't realise that until later
Note: We can add a The finite state machine approach is also interesting, probably even better to keep the simplicity. Wanna take a stab at it? |
Saw now that you added more events.
|
to detect the finishing things like |
@dignifiedquire in that case, there are no actions happening asynchronously. |
but they are now, because repo checks are async |
686099d
to
6fe86d5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lot's of cool stuff here, 👍 for the state machine approach!
Still needs some changes though.
start: false, | ||
EXPERIMENTAL: { | ||
pubsub: true | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why disabling PubSub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not disabled, it is enabled in the httpserver as far as I understand
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/cli/bin.js
Outdated
// Need to skip to avoid locking the daemon | ||
if (args[0] === 'daemon' || args[0] === 'init') { | ||
return cli.help().strict(false).completion().parse(args) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this more clear:
// commands that don't require a daemon/instance
// commands that require a daemon/instance
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, use same indentation in both
} | ||
cb(null, block) | ||
}) | ||
], callback) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the hashing is just a fallback if only a buffer is passed in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
bootstrap: config.Bootstrap | ||
mdns: get(config, 'Discovery.MDNS.Enabled'), | ||
webRTCStar: get(config, 'Discovery.webRTCStar.Enabled'), | ||
bootstrap: get(config, 'Bootstrap') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring a dependency to pick one property of an object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/core/components/pre-start.js
Outdated
@@ -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()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
strange identation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
very strange
src/core/status.js
Outdated
running: 'running', | ||
stopping: 'stopping', | ||
stopped: 'stopped' | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rechecked, this is just used to populate a var this._status
once and that const is not used anymore. ???
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the rest from some experimentation, going to delete it
cb() | ||
}) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is this now? Breaking API change? If so, update README
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know this was in the readme, this is not needed anymore internally that's why I removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't find it in the readme, are you sure this was documented somewhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, not on the readme, people are using it though - #776
Wanna finish that PR and merge it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
}) | ||
} catch (err) { | ||
return cb(err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is throwing? Errors should come on the error
event only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can throw if someone screwed up in the implementation, resulting in silent fail of the tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, let's add that comment, I want to avoid giving the idea that errors should be 'thrown' for contributors coming into the codebase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/go-ipfs-repo/datastore/LOG
Outdated
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.gitignore this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
test/go-ipfs-repo/version
Outdated
@@ -0,0 +1 @@ | |||
1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it really version 1?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm no not sure why that says version 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on disk it says 5 for me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, there were two
Seems that last thing is to rebase master onto this branch :) |
f486cac
to
d59dc43
Compare
3146ed4
to
b30fc99
Compare
I need to leave now for up to 3 hours, will come back online later. @dignifiedquire, let me know if you can check this. |
last missing piece ipfs/js-ipfs-repo#128 (hopefully) |
09bc36c
to
68d92b6
Compare
Closes #787
Closes #229
init
andready
) to handle the startup of a daemon more granularly (needs discussion probably)