Conversation
7dd8ff6 to
42a576e
Compare
9e5a1ff to
99b57f7
Compare
djdv
left a comment
There was a problem hiding this comment.
Minor comments, but otherwise looks good to me. Concise. :^)
d85d79c to
a232340
Compare
|
One remark I have, is that I noticed there's a lot of channels being used internally in floodsub. I wonder if it would be a good idea to have functions that return read only variants of them. But that's outside the scope of this PR. Just mentioning my thought. Bonus: video of a local test |
|
Handing off to @Stebalien for second pass |
core/coreapi/pubsub.go
Outdated
|
|
||
| if options.Discover { | ||
| go func() { | ||
| blk, err := api.core().Block().Put(ctx, strings.NewReader("floodsub:"+topic)) |
There was a problem hiding this comment.
This use of the context is a bit awkward. I'd expect the context to only apply to the lifetime of the Subscribe operation but it really applies until we're fully bootstrapped.
Maybe we should:
- Create an entirely new (independent) context.
- Cancel it when the subscription is closed.
(in that case, Subscribe may not need to take a context but it might be a good idea to leave it in the signature just in case)
core/coreapi/pubsub.go
Outdated
| defer cancel() | ||
|
|
||
| provs := n.Routing.FindProvidersAsync(ctx, cid, 10) | ||
| wg := &sync.WaitGroup{} |
There was a problem hiding this comment.
Nit: We don't actually need to allocate here. We can just use var wg sync.WaitGroup.
(not really important but still)
6067e52 to
9581396
Compare
9581396 to
0b1bd5f
Compare
|
Rebased. |
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
License: MIT Signed-off-by: Łukasz Magiera <magik6k@gmail.com>
0b1bd5f to
5618fed
Compare
Based on #4802
Replaces #4774
TODOs: