pkg/kademlia tests and restructuring

[coyle]

This change is 

[bryanchriswhite]

Reviewed 13 of 15 files at r1.
Review status: 13 of 15 files reviewed, 8 unresolved discussions (waiting on @coyle)

---

pkg/kademlia/kademlia.go, line 28 at r1 (raw file):

// Kademlia is an implementation of kademlia adhering to the DHT interface.
type Kademlia struct {
	rt             dht.RoutingTable

I think we would benefit from a less abbreviated name here; would routingTable be too long?

---

pkg/kademlia/kademlia.go, line 43 at r1 (raw file):

	}

	bb, err := convertProtoNodes(bootstrapNodes)

How about something like bkadNodes?

---

pkg/kademlia/kademlia.go, line 169 at r1 (raw file):

}

func convertNetworkNodes(n []*bkad.NetworkNode) []*proto.Node {

What do you think about convertBkadNodes or something?

---

pkg/kademlia/kademlia.go, line 178 at r1 (raw file):

}

func convertNetworkNode(v *bkad.NetworkNode) *proto.Node {

same ^

---

pkg/kademlia/kademlia_test.go, line 81 at r1 (raw file):

func TestBootstrap(t *testing.T) {
	dhts, bootNode := bootstrapTestNetwork(t, "127.0.0.1", "3000")

Maybe we want a more dynamic way of doing ports in tests after all?

---

pkg/kademlia/node.go, line 3 at r1 (raw file):

// Copyright (C) 2018 Storj Labs, Inc.
// See LICENSE for copying information.

Would it be weird for me to want to name this file "nodeid.go" or "node_id.go" or something, because we have a protobuf Node type?

---

pkg/overlay/service_test.go, line 25 at r1 (raw file):

	assert.NoError(t, err)

	srv := NewServer(nil, nil, nil, nil)

Please correct me if I'm mistaken but it looks to me like this only exercises integration of grpc and our protobuf a bit. I'm not saying that's not useful; however, I think the name is unrepresentative of what it's doing.

---

pkg/overlay/service_test.go, line 52 at r1 (raw file):

	o := Service{}
	ctx, cf := context.WithTimeout(context.Background(), 3*time.Second)

Unless you feel strongly about cf I'd like to suggest cancel as an alternative; I think we should be consistent regardless though.

---

Comments from Reviewable

[bryanchriswhite]

Reviewed 1 of 15 files at r1.
Review status: 14 of 15 files reviewed, 8 unresolved discussions (waiting on @coyle)

---

Comments from Reviewable

[bryanchriswhite]

Reviewed 1 of 15 files at r1.
Review status: all files reviewed, 8 unresolved discussions (waiting on @coyle)

---

Comments from Reviewable

[coyle]

Review status: all files reviewed, 8 unresolved discussions (waiting on @coyle)

---

pkg/kademlia/kademlia.go, line 28 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

I think we would benefit from a less abbreviated name here; would routingTable be too long?

I'm ok with this change but since it's a private field on the struct, idiomatic go is to favor short names.

---

pkg/kademlia/kademlia.go, line 43 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

How about something like bkadNodes?

you din't like the short name ?

---

pkg/kademlia/kademlia.go, line 169 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

What do you think about convertBkadNodes or something?

They are NetworkNodes I;m just calling the import path bkad

---

pkg/kademlia/kademlia.go, line 178 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

same ^

same reply

---

pkg/kademlia/kademlia_test.go, line 81 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

Maybe we want a more dynamic way of doing ports in tests after all?

yea, I need to figure out why they aren't closing but thought you had done port selection in your PR, so was waiting for that.

---

pkg/kademlia/node.go, line 3 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

Would it be weird for me to want to name this file "nodeid.go" or "node_id.go" or something, because we have a protobuf Node type?

I'm ok with changing the name

---

pkg/overlay/service_test.go, line 25 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

Please correct me if I'm mistaken but it looks to me like this only exercises integration of grpc and our protobuf a bit. I'm not saying that's not useful; however, I think the name is unrepresentative of what it's doing.

I need to add more tests. I got distracted chasing down that bug yesterday. I'll do that now

---

pkg/overlay/service_test.go, line 52 at r1 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

Unless you feel strongly about cf I'd like to suggest cancel as an alternative; I think we should be consistent regardless though.

cancel is good yes, let's be consistent

---

Comments from Reviewable

[bryanchriswhite]

Review status: all files reviewed, 8 unresolved discussions (waiting on @bryanchriswhite)

---

pkg/kademlia/kademlia.go, line 169 at r1 (raw file):

Previously, coyle (Dennis Coyle) wrote…

They are NetworkNodes I;m just calling the import path bkad

Sure; although, we hide that fact in our encapsulation.

I don't feel strongly about it but I was just thinking that since NetworkNode is a bkad thing and not used anywhere else, it might be less confusing to call it something like convertBkadNodes.

---

pkg/kademlia/kademlia_test.go, line 81 at r1 (raw file):

Previously, coyle (Dennis Coyle) wrote…

yea, I need to figure out why they aren't closing but thought you had done port selection in your PR, so was waiting for that.

What do you mean "why they aren't closing"? Travis is passing and the tests worked on my machine.

With JT's feedback I was able to factor out the port stuff because for the things I was testing I was able to pass "0" for the port which ultimately gets handed off to a listen call or something which gets turned into an available port. In this case though, you need to know what ports will be used for the test or at least be able to get that information from the stuff in memory if you were to use "0" as well.

---

Comments from Reviewable

[bryanchriswhite]

Review status: 20 of 22 files reviewed, 1 unresolved discussion (waiting on @bryanchriswhite and @kaloyan-raev)

---

pkg/overlay/overlay.go, line 70 at r3 (raw file):

Previously, bryanchriswhite (Bryan White) wrote…

Haha, that makes sense

Resolving in advance

---

Comments from Reviewable

[bryanchriswhite]

Reviewed 2 of 9 files at r3.
Review status: all files reviewed, 1 unresolved discussion (waiting on @kaloyan-raev)

---

Comments from Reviewable

[jtolio]

Review status: 21 of 22 files reviewed, 4 unresolved discussions (waiting on @bryanchriswhite, @kaloyan-raev, and @coyle)

---

pkg/kademlia/kademlia.go, line 161 at r4 (raw file):

	}

	go k.dht.Listen()

this is fine but my comment about making a ListenAndServe method was more so that the caller of ListenAndServe had control over kicking off a goroutine or not. Personally I'd expect a ListenAndServe method to not return until it's done listening and serving

---

pkg/kademlia/kademlia.go, line 219 at r4 (raw file):

	addr := "bootstrap.storj.io:8080"
	if ip != "" && port != "" {
		addr = ip + ":" + port

reminder about net.JoinHostPort for ipv6 support. honestly putting colons in ipv6 addresses was a terrible mistake

---

pkg/kademlia/test_utils.go, line 1 at r4 (raw file):

package kademlia

copyright

---

Comments from Reviewable

[jtolio]

looking good!

---

Review status: 21 of 22 files reviewed, 4 unresolved discussions (waiting on @bryanchriswhite, @kaloyan-raev, and @coyle)

---

Comments from Reviewable

[coyle]

Reviewed 3 of 9 files at r3, 1 of 9 files at r6.
Review status: 15 of 22 files reviewed, 2 unresolved discussions (waiting on @bryanchriswhite and @coyle)

---

pkg/kademlia/kademlia.go, line 161 at r4 (raw file):

Previously, jtolds (JT Olio) wrote…

this is fine but my comment about making a ListenAndServe method was more so that the caller of ListenAndServe had control over kicking off a goroutine or not. Personally I'd expect a ListenAndServe method to not return until it's done listening and serving

We need it listening before we can call bootstrap, at least while we are using bkad stuff. I 100% agree though. as soon as we restructure how we are doing things, as we move away from bkad. I will make this work as expected.

---

pkg/kademlia/kademlia.go, line 219 at r4 (raw file):

Previously, jtolds (JT Olio) wrote…

reminder about net.JoinHostPort for ipv6 support. honestly putting colons in ipv6 addresses was a terrible mistake

keep forgetting about ipv6 thanks for the reminder

Done.

---

Comments from Reviewable

[coyle]

Reviewed 1 of 15 files at r1, 7 of 9 files at r6.
Review status: all files reviewed, 2 unresolved discussions (waiting on @jtolds)

---

Comments from Reviewable

[jtolio]

Review status: all files reviewed, 2 unresolved discussions (waiting on @coyle)

---

pkg/dht/dht.go, line 15 at r6 (raw file):

// NodeID is the unique identifer used for Nodes in the DHT
type NodeID interface {
	String() string

i assume this returns base58 or something?

---

pkg/dht/dht.go, line 16 at r6 (raw file):

type NodeID interface {
	String() string
	Bytes() []byte

and this returns binary?

---

Comments from Reviewable

[jtolio]

---

Review status: all files reviewed, 2 unresolved discussions (waiting on @coyle)

---

Comments from Reviewable

[jtolio]

how do i mark it green? i can't figure out reviewable lol

---

Review status: all files reviewed, 2 unresolved discussions (waiting on @coyle)

---

Comments from Reviewable

[jtolio]

oh wait i think i did it

---

Review status: all files reviewed, 2 unresolved discussions (waiting on @coyle)

---

Comments from Reviewable

[coyle]

Review status: all files reviewed, 2 unresolved discussions (waiting on @coyle)

---

pkg/dht/dht.go, line 15 at r6 (raw file):

Previously, jtolds (JT Olio) wrote…

i assume this returns base58 or something?

correct. I'll add a comment to make this clear

---

pkg/dht/dht.go, line 16 at r6 (raw file):

Previously, jtolds (JT Olio) wrote…

and this returns binary?

correct. I'll add a comment to make this clear

---

Comments from Reviewable