-
Notifications
You must be signed in to change notification settings - Fork 221
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
Rename IpfsDHT to Node #223
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -37,9 +37,11 @@ var log = logging.Logger("dht") | |
// collect members of the routing table. | ||
const NumBootstrapQueries = 5 | ||
|
||
// IpfsDHT is an implementation of Kademlia with S/Kademlia modifications. | ||
type IpfsDHT = Node | ||
|
||
// Node is an implementation of Kademlia with S/Kademlia modifications. | ||
// It is used to implement the base IpfsRouting module. | ||
type IpfsDHT struct { | ||
type Node struct { | ||
host host.Host // the network services we need | ||
self peer.ID // Local peer (yourself) | ||
peerstore pstore.Peerstore // Peer Registry | ||
|
@@ -65,7 +67,7 @@ type IpfsDHT struct { | |
} | ||
|
||
// New creates a new DHT with the specified host and options. | ||
func New(ctx context.Context, h host.Host, options ...opts.Option) (*IpfsDHT, error) { | ||
func New(ctx context.Context, h host.Host, options ...opts.Option) (*Node, error) { | ||
var cfg opts.Options | ||
if err := cfg.Apply(append([]opts.Option{opts.Defaults}, options...)...); err != nil { | ||
return nil, err | ||
|
@@ -95,7 +97,7 @@ func New(ctx context.Context, h host.Host, options ...opts.Option) (*IpfsDHT, er | |
// NewDHT creates a new DHT object with the given peer as the 'local' host. | ||
// IpfsDHT's initialized with this function will respond to DHT requests, | ||
// whereas IpfsDHT's initialized with NewDHTClient will not. | ||
func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | ||
func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about we take the plunge and rename this func to
We'd need to introduce a method alias, though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I concur. I also want to rename NewDHTClient to NewClientNode, or NewPassiveNode. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's not unnecessarily break existing code. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alias would be added for a period for backwards compatibility until the next major version. |
||
dht, err := New(ctx, h, opts.Datastore(dstore)) | ||
if err != nil { | ||
panic(err) | ||
|
@@ -107,15 +109,15 @@ func NewDHT(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | |
// host. IpfsDHT clients initialized with this function will not respond to DHT | ||
// requests. If you need a peer to respond to DHT requests, use NewDHT instead. | ||
// NewDHTClient creates a new DHT object with the given peer as the 'local' host | ||
func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching) *IpfsDHT { | ||
func NewDHTClient(ctx context.Context, h host.Host, dstore ds.Batching) *Node { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As per the above comment, this could become:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that while you can return private types and still operate on them, they're concealed from docs and a bit dubious. Unfortunately being able to arbitrarily instantiate any public type without going through a constructor is just a Go wart, and not worth trying to evade. |
||
dht, err := New(ctx, h, opts.Datastore(dstore), opts.Client(true)) | ||
if err != nil { | ||
panic(err) | ||
} | ||
return dht | ||
} | ||
|
||
func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []protocol.ID) *IpfsDHT { | ||
func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []protocol.ID) *Node { | ||
rt := kb.NewRoutingTable(KValue, kb.ConvertPeerID(h.ID()), time.Minute, h.Peerstore()) | ||
|
||
cmgr := h.ConnManager() | ||
|
@@ -126,7 +128,7 @@ func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []p | |
cmgr.UntagPeer(p, "kbucket") | ||
} | ||
|
||
return &IpfsDHT{ | ||
return &Node{ | ||
datastore: dstore, | ||
self: h.ID(), | ||
peerstore: h.Peerstore(), | ||
|
@@ -141,7 +143,7 @@ func makeDHT(ctx context.Context, h host.Host, dstore ds.Batching, protocols []p | |
} | ||
|
||
// putValueToPeer stores the given key/value pair at the peer 'p' | ||
func (dht *IpfsDHT) putValueToPeer(ctx context.Context, p peer.ID, rec *recpb.Record) error { | ||
func (dht *Node) putValueToPeer(ctx context.Context, p peer.ID, rec *recpb.Record) error { | ||
|
||
pmes := pb.NewMessage(pb.Message_PUT_VALUE, rec.Key, 0) | ||
pmes.Record = rec | ||
|
@@ -165,7 +167,7 @@ var errInvalidRecord = errors.New("received invalid record") | |
// key. It returns either the value or a list of closer peers. | ||
// NOTE: It will update the dht's peerstore with any new addresses | ||
// it finds for the given peer. | ||
func (dht *IpfsDHT) getValueOrPeers(ctx context.Context, p peer.ID, key string) (*recpb.Record, []*pstore.PeerInfo, error) { | ||
func (dht *Node) getValueOrPeers(ctx context.Context, p peer.ID, key string) (*recpb.Record, []*pstore.PeerInfo, error) { | ||
|
||
pmes, err := dht.getValueSingle(ctx, p, key) | ||
if err != nil { | ||
|
@@ -200,7 +202,7 @@ func (dht *IpfsDHT) getValueOrPeers(ctx context.Context, p peer.ID, key string) | |
} | ||
|
||
// getValueSingle simply performs the get value RPC with the given parameters | ||
func (dht *IpfsDHT) getValueSingle(ctx context.Context, p peer.ID, key string) (*pb.Message, error) { | ||
func (dht *Node) getValueSingle(ctx context.Context, p peer.ID, key string) (*pb.Message, error) { | ||
meta := logging.LoggableMap{ | ||
"key": key, | ||
"peer": p, | ||
|
@@ -224,7 +226,7 @@ func (dht *IpfsDHT) getValueSingle(ctx context.Context, p peer.ID, key string) ( | |
} | ||
|
||
// getLocal attempts to retrieve the value from the datastore | ||
func (dht *IpfsDHT) getLocal(key string) (*recpb.Record, error) { | ||
func (dht *Node) getLocal(key string) (*recpb.Record, error) { | ||
log.Debugf("getLocal %s", key) | ||
rec, err := dht.getRecordFromDatastore(mkDsKey(key)) | ||
if err != nil { | ||
|
@@ -243,7 +245,7 @@ func (dht *IpfsDHT) getLocal(key string) (*recpb.Record, error) { | |
|
||
// getOwnPrivateKey attempts to load the local peers private | ||
// key from the peerstore. | ||
func (dht *IpfsDHT) getOwnPrivateKey() (ci.PrivKey, error) { | ||
func (dht *Node) getOwnPrivateKey() (ci.PrivKey, error) { | ||
sk := dht.peerstore.PrivKey(dht.self) | ||
if sk == nil { | ||
log.Warningf("%s dht cannot get own private key!", dht.self) | ||
|
@@ -253,7 +255,7 @@ func (dht *IpfsDHT) getOwnPrivateKey() (ci.PrivKey, error) { | |
} | ||
|
||
// putLocal stores the key value pair in the datastore | ||
func (dht *IpfsDHT) putLocal(key string, rec *recpb.Record) error { | ||
func (dht *Node) putLocal(key string, rec *recpb.Record) error { | ||
log.Debugf("putLocal: %v %v", key, rec) | ||
data, err := proto.Marshal(rec) | ||
if err != nil { | ||
|
@@ -266,13 +268,13 @@ func (dht *IpfsDHT) putLocal(key string, rec *recpb.Record) error { | |
|
||
// Update signals the routingTable to Update its last-seen status | ||
// on the given peer. | ||
func (dht *IpfsDHT) Update(ctx context.Context, p peer.ID) { | ||
func (dht *Node) Update(ctx context.Context, p peer.ID) { | ||
log.Event(ctx, "updatePeer", p) | ||
dht.routingTable.Update(p) | ||
} | ||
|
||
// FindLocal looks for a peer with a given ID connected to this dht and returns the peer and the table it was found in. | ||
func (dht *IpfsDHT) FindLocal(id peer.ID) pstore.PeerInfo { | ||
func (dht *Node) FindLocal(id peer.ID) pstore.PeerInfo { | ||
switch dht.host.Network().Connectedness(id) { | ||
case inet.Connected, inet.CanConnect: | ||
return dht.peerstore.PeerInfo(id) | ||
|
@@ -282,7 +284,7 @@ func (dht *IpfsDHT) FindLocal(id peer.ID) pstore.PeerInfo { | |
} | ||
|
||
// findPeerSingle asks peer 'p' if they know where the peer with id 'id' is | ||
func (dht *IpfsDHT) findPeerSingle(ctx context.Context, p peer.ID, id peer.ID) (*pb.Message, error) { | ||
func (dht *Node) findPeerSingle(ctx context.Context, p peer.ID, id peer.ID) (*pb.Message, error) { | ||
eip := log.EventBegin(ctx, "findPeerSingle", | ||
logging.LoggableMap{ | ||
"peer": p, | ||
|
@@ -304,7 +306,7 @@ func (dht *IpfsDHT) findPeerSingle(ctx context.Context, p peer.ID, id peer.ID) ( | |
} | ||
} | ||
|
||
func (dht *IpfsDHT) findProvidersSingle(ctx context.Context, p peer.ID, key cid.Cid) (*pb.Message, error) { | ||
func (dht *Node) findProvidersSingle(ctx context.Context, p peer.ID, key cid.Cid) (*pb.Message, error) { | ||
eip := log.EventBegin(ctx, "findProvidersSingle", p, key) | ||
defer eip.Done() | ||
|
||
|
@@ -323,13 +325,13 @@ func (dht *IpfsDHT) findProvidersSingle(ctx context.Context, p peer.ID, key cid. | |
} | ||
|
||
// nearestPeersToQuery returns the routing tables closest peers. | ||
func (dht *IpfsDHT) nearestPeersToQuery(pmes *pb.Message, count int) []peer.ID { | ||
func (dht *Node) nearestPeersToQuery(pmes *pb.Message, count int) []peer.ID { | ||
closer := dht.routingTable.NearestPeers(kb.ConvertKey(string(pmes.GetKey())), count) | ||
return closer | ||
} | ||
|
||
// betterPeersToQuery returns nearestPeersToQuery, but if and only if closer than self. | ||
func (dht *IpfsDHT) betterPeersToQuery(pmes *pb.Message, p peer.ID, count int) []peer.ID { | ||
func (dht *Node) betterPeersToQuery(pmes *pb.Message, p peer.ID, count int) []peer.ID { | ||
closer := dht.nearestPeersToQuery(pmes, count) | ||
|
||
// no node? nil | ||
|
@@ -359,21 +361,21 @@ func (dht *IpfsDHT) betterPeersToQuery(pmes *pb.Message, p peer.ID, count int) [ | |
} | ||
|
||
// Context return dht's context | ||
func (dht *IpfsDHT) Context() context.Context { | ||
func (dht *Node) Context() context.Context { | ||
return dht.ctx | ||
} | ||
|
||
// Process return dht's process | ||
func (dht *IpfsDHT) Process() goprocess.Process { | ||
func (dht *Node) Process() goprocess.Process { | ||
return dht.proc | ||
} | ||
|
||
// Close calls Process Close | ||
func (dht *IpfsDHT) Close() error { | ||
func (dht *Node) Close() error { | ||
return dht.proc.Close() | ||
} | ||
|
||
func (dht *IpfsDHT) protocolStrs() []string { | ||
func (dht *Node) protocolStrs() []string { | ||
pstrs := make([]string, len(dht.protocols)) | ||
for idx, proto := range dht.protocols { | ||
pstrs[idx] = string(proto) | ||
|
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.
If we're keeping this struct public, I'd rather prefix it with DHT, and remove the DHT from the functions below.
But actually I wonder if there's any use case to keep Node public; I suspect no. In that case I'd be happy with an unqualified
node
type demoted to private scope.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 interface is wider than the routing interface; we would need a new interface for the DHT.
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.
To my knowledge, returning a private type allows consumers to access all its public methods, so you don't need to access them through an interface. In this case, the benefit is that it disallows constructing a struct by itself, and it doesn't leak the unqualified
Node
type to the public namespace.BTW – I generally agree that there may be more interfaces to extract.
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.
You can't construct a variable of it's type though, so you can't assign it anywhere which makes it borderline useless.
Note that the interface of the DHT is wider than any extant interface we have.
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, you can assign it to a local variable with the
:=
operator, but to pass it around in function arguments, you need to declare those args as interfaces. I'd argue that's the correct way, though.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 think it should be a public type, and that type should be returned explicitly by its constructors. See the Go idiom on returning concrete types and accepting interfaces.