-
Notifications
You must be signed in to change notification settings - Fork 337
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
Fallback to bootnodes #558
Conversation
Hi Petar, could you write a description and/or connect this one with an issue? |
@Eknir Sure, done! |
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.
minor comments, otherwise lgtm
} | ||
} | ||
} | ||
|
||
func (k *Kad) Start(ctx context.Context) 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 not execute this code as part of the kademlia constructor?
}(a) | ||
} | ||
wg.Wait() | ||
if err := kad.Start(p2pCtx); err != nil { |
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.
not sure if i understand why this is needed as a discrete step in the kademlia start up sequence? is there anything preventing the address book to populate kademlia as part of the kademlia constructor?
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 guess not, but I had a weird feeling when using it that way. Because, we need kademlia as a dependency on some other components so it is created pretty "early", while this kind of logic where there is an ongoing loop looks more like a "service" to me. Especially now with connect to bootnodes as well. To put it simple, I liked the idea of being able to instantiate other components with kademlia, but not start the logic that is driving rest of the system as well, such as connecting, notifying, etc... LMT if this makes sense?
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 does give more control to the process, indeed. Even it may seem an overhead, I think that it is a safe approach.
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.
that's fine, but the manage
loop is started in the constructor. it falls under this same case, so either pull the manage goroutine start to this function, or have both in the ctor. WDYT?
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.
Sure, done! Put them both in Start()
.
pkg/hive/hive.go
Outdated
logger logging.Logger | ||
streamer p2p.Streamer | ||
addressBook addressbook.GetPutter | ||
peerAddedHandler func(context.Context, ...swarm.Address) 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.
A very minor naming suggestion addPeersHandler
?
pkg/kademlia/doc.go
Outdated
@@ -15,7 +15,7 @@ was persisted in the address book and the node has been restarted). | |||
|
|||
So the information has been changed, and potentially upon disconnection, | |||
the depth can travel to a shallower depth in result. | |||
If a peer gets added through AddPeer, this does not necessarily infer | |||
If a peer gets added through AddPeers , this does not necessarily infer |
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.
Remove additional whitespace.
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, did not save all files after auto rename 🙈
pkg/node/node.go
Outdated
logger.Debugf("multiaddress fail %s: %v", a, err) | ||
logger.Warningf("connect to bootnode %s", a) |
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.
These log messages should be adjusted.
}(a) | ||
} | ||
wg.Wait() | ||
if err := kad.Start(p2pCtx); err != nil { |
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 does give more control to the process, indeed. Even it may seem an overhead, I think that it is a safe approach.
pkg/topology/mock/mock.go
Outdated
@@ -16,14 +16,14 @@ type mock struct { | |||
peers []swarm.Address | |||
closestPeer swarm.Address | |||
closestPeerErr error | |||
addPeerErr error | |||
AddPeersErr 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.
It looks to me that this does not have to be exported.
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, that was a rename. This is the last time I used rename all functionality, I am apparently very bad at using 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.
Thanks @pradovic
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.
hvala
handle bootnodes in kademlia |
#521
I have left
Start()
method only on kademlia for now, so I did not extract any extra interfaces. This is probably ok, as there is no other implementation of topology driver at the moment, and full connectivity does not need it.