Skip to content
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.

Sign up for GitHub

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

Jump to bottom

coreapi: PubSub API #4805

Merged
merged 6 commits into from
Oct 5, 2018
Merged

coreapi: PubSub API #4805

merged 6 commits into from
Oct 5, 2018

Conversation

magik6k
Copy link
Member

@magik6k magik6k commented Mar 10, 2018 •
edited by Stebalien
Loading

Based on #4802

Replaces #4774

TODOs:

@magik6k magik6k requested a review from Kubuxu as a code owner March 10, 2018 18:35
@ghost ghost assigned magik6k Mar 10, 2018
@ghost ghost added the status/in-progress In progress label Mar 10, 2018
@magik6k magik6k added topic/core-api Topic core-api status/blocked Unable to be worked further until needs are met labels Mar 11, 2018
@magik6k magik6k mentioned this pull request Mar 12, 2018
Open
51 tasks
@magik6k magik6k removed the status/blocked Unable to be worked further until needs are met label Mar 23, 2018
@magik6k
Copy link
Member Author

magik6k commented Mar 24, 2018

#4807 and #4804 need to get merged first

@magik6k magik6k added the status/blocked Unable to be worked further until needs are met label Mar 24, 2018
@Stebalien Stebalien removed the status/blocked Unable to be worked further until needs are met label Sep 11, 2018
@magik6k magik6k force-pushed the feat/coreapi/pubsub branch 3 times, most recently from 9e5a1ff to 99b57f7 Compare September 11, 2018 09:20
@Stebalien Stebalien requested a review from djdv September 12, 2018 21:24
djdv
djdv suggested changes Sep 13, 2018
View reviewed changes
Copy link
Contributor

@djdv djdv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comments, but otherwise looks good to me. Concise. :^)

core/coreapi/interface/pubsub.go Outdated Show resolved Hide resolved
core/coreapi/pubsub.go Show resolved Hide resolved
core/commands/pubsub.go Show resolved Hide resolved
@magik6k magik6k requested a review from djdv September 26, 2018 18:53
djdv
djdv previously approved these changes Sep 27, 2018
View reviewed changes
@djdv
Copy link
Contributor

djdv commented Sep 27, 2018

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
https://ipfs.io/ipfs/QmRbztNNKLzJgPDsPS2Di8oMb3pYLAtRJTEuFF5dC6tkXF/Desktop%202018.09.27%20-%2013.14.36.12.mp4

@djdv
Copy link
Contributor

djdv commented Sep 27, 2018

Handing off to @Stebalien for second pass

Stebalien
Stebalien reviewed Sep 27, 2018
View reviewed changes
core/coreapi/interface/pubsub.go Show resolved Hide resolved
core/coreapi/interface/pubsub.go Show resolved Hide resolved
core/coreapi/pubsub.go Outdated

if options.Discover {
go func() {
blk, err := api.core().Block().Put(ctx, strings.NewReader("floodsub:"+topic))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Create an entirely new (independent) context.
  2. 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{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: We don't actually need to allocate here. We can just use var wg sync.WaitGroup.

(not really important but still)

@magik6k magik6k force-pushed the feat/coreapi/pubsub branch 2 times, most recently from 6067e52 to 9581396 Compare October 2, 2018 11:18
@magik6k magik6k requested a review from Stebalien October 2, 2018 12:05
@magik6k
Copy link
Member Author

magik6k commented Oct 2, 2018

Rebased.

@magik6k @Stebalien
coreapi: pubsub interface
3d3bedc 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k @Stebalien
coreapi: implement pubsub api
51bb9d6 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k @Stebalien
coreapi pubsub: add tests
d3f3afa 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k @Stebalien
pubsub cmd: switch to coreapi
d6aa952 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k @Stebalien
coreapi pubsub: fix review nits
18d5999 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@magik6k @Stebalien
coreapi pubsub: better ctx for connectToPubSubPeers
5618fed 
License: MIT
Signed-off-by: Łukasz Magiera <[email protected]>
@Stebalien Stebalien merged commit 41a7388 into master Oct 5, 2018
@Stebalien Stebalien deleted the feat/coreapi/pubsub branch October 5, 2018 18:57
@ghost ghost removed the status/in-progress In progress label Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Stebalien Stebalien Stebalien approved these changes

@djdv djdv djdv left review comments

@Kubuxu Kubuxu Awaiting requested review from Kubuxu

Assignees

@magik6k magik6k

Labels
topic/core-api Topic core-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@magik6k @djdv @Stebalien