-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
replace nodebuilder with a nicer interface #1572
Conversation
cd3d6df
to
686f5a3
Compare
@jbenet let me know what you think of this interface. If its what we want, i'll go ahead and switch over a bunch of the codebase to use it instead of the old way. |
i like it :) |
alright! Choo Choo! time for the refactor train :D |
Also, have been wondering, why the https://github.com/ipfs/go-ipfs/pull/1572/files#diff-54f2c4a5481de30b8ccb59ad5b6588b3L242? Sounds redundant with something that sounds "NodeBuilder". Can't the node be asserted to exist at the creation of the |
bcbdeca
to
ab486fa
Compare
interesting failure here: https://travis-ci.org/ipfs/go-ipfs/jobs/75662605 |
} | ||
|
||
// TODO: shrink this function as much as possible, moving logic into NewNode | ||
func NewMockNodeOnNet(ctx context.Context, mn mocknet.Mocknet) (*core.IpfsNode, error) { |
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.
Since it proved difficult to combine mocknet and the node constructor i was working on, i have this method here as a bit of a shim until i figure out the right way to do it.
ab486fa
to
63fc7bb
Compare
much better :) RFCR |
The simplest case of getting an offline node for testing something is now:
|
} | ||
n.Resolver = &path.Resolver{DAG: n.DAG} | ||
|
||
success = 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.
should be just n.teardown()
(no defer) since it is only used for this path.
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.
no, there are three different places we could return early, triggering the need for a teardown.
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 c, I got this reversed. teardown is not called only for this path.
This can be used for res.SetError
defer func(){
if err != nil {
res.SetError(err, cmds.ErrNormal)
}
}()
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 function is a scary still, and the success/defer
stuff is not easy to modify safely (error prone).
how about this (it should work exactly):
func cfgDefaults(cfg *BuildCfg) (*BuildCfg, error) {
if cfg == nil {
cfg = new(BuildCfg)
}
err := cfg.fillDefaults()
if err != nil {
return nil, err
}
}
func NewNode(ctx context.Context, cfg *BuildCfg) (*IpfsNode, error) {
cfg, err := cfgDefaults(cfg)
if err != nil {
return nil, err
}
n := &IpfsNode{
mode: offlineMode,
Repo: cfg.Repo,
ctx: ctx,
Peerstore: peer.NewPeerstore(),
}
if cfg.Online {
n.mode = onlineMode
}
// TODO: this is a weird circular-ish dependency, rework it
n.proc = goprocessctx.WithContextAndTeardown(ctx, n.teardown)
if err := tryNodeSetup(ctx, cfg, n); err != nil {
n.Close()
return nil, err
}
return n, nil
}
func tryNodeSetup(ctx context.Context, cfg *BuildCfg, n *IpfsNode) error {
// setup local peer ID (private key is loaded in online setup)
if err := n.loadID(); err != nil {
return nil, err
}
bs := bstore.NewBlockstore(n.Repo.Datastore())
n.Blockstore, err = bstore.WriteCached(bs, kSizeBlockstoreWriteCache)
if err != nil {
return nil, err
}
if cfg.Online {
do := setupDiscoveryOption(n.Repo.Config().Discovery)
if err := n.startOnlineServices(ctx, cfg.Routing, cfg.Host, do); err != nil {
return nil, err
}
} else {
n.Exchange = offline.Exchange(n.Blockstore)
}
n.Blocks = bserv.New(n.Blockstore, n.Exchange)
n.DAG = dag.NewDAGService(n.Blocks)
n.Pinning, err = pin.LoadPinner(n.Repo.Datastore(), n.DAG)
if err != nil {
// TODO: we should move towards only running 'NewPinner' explicity on
// node init instead of implicitly here as a result of the pinner keys
// not being found in the datastore.
// this is kinda sketchy and could cause data loss
n.Pinning = pin.NewPinner(n.Repo.Datastore(), n.DAG)
}
n.Resolver = &path.Resolver{DAG: n.DAG}
return nil
}
Note: call n.Close()
, or even n.proc.Close()
. do not call n.teardown()
directly!! or it may be called twice and break semantics (maybe panic)
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, i like that better.
@whyrusleeping there is still one use left of |
@whyrusleeping should i take another look here? |
@jbenet yeah, i think i got it right now. |
online: false, | ||
routing: DHTOption, | ||
peerhost: DefaultHostOption, | ||
func (cfg *BuildCfg) fillDefaults() error { |
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 should error when:
cfg.Repo != nil && cfg.NilRepo
peers := mn.Peers() | ||
if len(peers) < numPeers { | ||
t.Fatal(errors.New("test initialization error")) | ||
} |
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 removing this? it can catch serious errors that might otherwise drive people mad.
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.
(maybe it just belongs elsewhere)
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.
Because no peers are created by the mocknet constructor, i'm creating them manually in a loop and adding them.
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 ok!
👍 i think this is way cleaner. comments above |
373d334
to
3e2b7ae
Compare
This would mean It is part of this PR, I think, because the |
@jbenet yeah, i think we should do away with the |
Ok. @whyrusleeping this needs rebasing i think |
i think that's the last thing, right? this should be good to go |
3e2b7ae
to
969b8b8
Compare
rebased! |
@whyrusleeping do all commits pass? either |
969b8b8
to
a12661b
Compare
squashed. |
@whyrusleeping conflicts? |
License: MIT Signed-off-by: Jeromy <[email protected]> use NewNode instead of NewIPFSNode in most of the codebase License: MIT Signed-off-by: Jeromy <[email protected]> make mocknet work with node constructor better License: MIT Signed-off-by: Jeromy <[email protected]> finish cleanup of old construction method License: MIT Signed-off-by: Jeromy <[email protected]> blockservice.New doesnt return an error anymore License: MIT Signed-off-by: Jeromy <[email protected]> break up node construction into separate function License: MIT Signed-off-by: Jeromy <[email protected]> add error case to default filling on node constructor License: MIT Signed-off-by: Jeromy <[email protected]>
a12661b
to
94000e6
Compare
@jbenet conflicts resolved. |
replace nodebuilder with a nicer interface
a less extreme version of #1557 that uses a
BuildCfg
to set all the same options.License: MIT
Signed-off-by: Jeromy [email protected]