Implement Accelerated DHT #45
Conversation
dennis-tra
force-pushed
the
v2-issue-7-accelerated-dht
branch
from
b55f2db
to
de85493
Compare
dennis-tra
force-pushed
the
v2-issue-7-accelerated-dht
branch
3 times, most recently
from
b9d84a9
to
57d2949
Compare
dennis-tra
force-pushed
the
v2-issue-7-accelerated-dht
branch
from
6a24dd1
to
d99b889
Compare
| @@ -0,0 +1,15 @@ | |||
| package zikade | |||
|
|
|||
| //func TestNewFullRT(t *testing.T) { | |||
There was a problem hiding this comment.
write at least one test for FullRT
dennis-tra
force-pushed
the
v2-issue-7-accelerated-dht
branch
from
6d4043c
to
e6118a3
Compare
| "github.com/plprobelab/zikade/pb" | ||
| ) | ||
|
|
||
| type FullRT struct { |
There was a problem hiding this comment.
It doesn't make much sense to me to call this FullRT. It seems to just be following the same naming as the go-libp2p-kad-dht implementation.
It seems to me that this is really a specialised routing table population strategy for the DHT. Can we make it an option on the normal DHT type?
There was a problem hiding this comment.
Yeah, I also wasn't sure about that. Though it's more than just a specialized routing table population strategy. The routing.Routing implementation also behaves quite differently.
If we put an option on the DHT type we'd need to branch into either the default routing.Routing implementation or the fullRT routing.Routing implementation which I think is not super elegant. I don't have a better idea though :/
| } | ||
|
|
||
| for j := 0; j < c.cfg.MaxCPL; j++ { | ||
| target, err := c.cplFn(node.Key(), j) |
There was a problem hiding this comment.
Rename this to targetNode since job.target is a key
| return &StateCrawlIdle{} | ||
| } | ||
|
|
||
| if len(c.info.waiting) >= c.cfg.MaxCPL*c.cfg.Concurrency { |
There was a problem hiding this comment.
| if len(c.info.waiting) >= c.cfg.MaxCPL*c.cfg.Concurrency { | |
| if len(c.info.waiting) >= c.cfg.Concurrency { |
Concurrency is the maximum number of concurrent requests, but the original code is sending 16 times as many
There was a problem hiding this comment.
Sending 16 requests is intended because each request contains (should contain) a different target key for which we want to know the 20 closest nodes that the other peer knows. This is the strategy for effectively fetching the entire routing table of a remote peer.
There was a problem hiding this comment.
Concurrency should be the maximum number of in-flight requests. The 16 is irrelevant here, since we are checking how many are currently in-flight. As it stands if the user specifies concurrency of 200 then they will actually end up with 3200 concurrent requests.
| } | ||
|
|
||
| span.SetAttributes( | ||
| attribute.Int(prefix+"_todo", len(c.info.todo)), |
There was a problem hiding this comment.
Report these as metric gauges too
| } | ||
|
|
||
| // clear info to indicate that we're idle | ||
| c.info = nil |
There was a problem hiding this comment.
Add a metrics gauge that is 1 when the crawl is running and 0 otherwise
| c.info = nil | ||
|
|
||
| return &StateCrawlFinished[K, N]{ | ||
| Nodes: nodes, |
There was a problem hiding this comment.
This could contain thousands of nodes, do we need to return this? Could include stats instead: number found, number of errors etc.
| cpls map[string]int | ||
| waiting map[string]N | ||
| success map[string]N | ||
| failed map[string]N |
There was a problem hiding this comment.
We don't used the actual list of failures or errors. It would be less memory to keep a counts instead. If we don't return the (potentially very large) list of successful nodes then we could keep a count instead too
| } | ||
|
|
||
| newJob := crawlJob[K, N]{ | ||
| node: node, |
There was a problem hiding this comment.
What prevents us from crawling a node multiple times for the same target? Nodes A and B could both return node C in their list of nodes closer to target T.
There was a problem hiding this comment.
Ah, I see that's what cpls map is for. Can you add comments on the fields?
There was a problem hiding this comment.
Actually, thinking about it, couldn't we end up asking the same node for different targets with the same CPL? The CPL function returns a random key in with the given CPL so a node could be asked to crawl the same CPL more than once with different keys
There was a problem hiding this comment.
I think you're right 🤔 Let me write a test for it and assert exactly that 👍
Context: #7