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.

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

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

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
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
@magik6k magik6k force-pushed the feat/coreapi/pubsub branch from 7dd8ff6 to 42a576e Compare March 24, 2018 22:29
@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
@magik6k magik6k requested a review from Stebalien September 11, 2018 11:01
@Stebalien Stebalien requested a review from djdv September 12, 2018 21:24
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 force-pushed the feat/coreapi/pubsub branch from d85d79c to a232340 Compare September 25, 2018 15:01
@magik6k magik6k requested a review from djdv September 26, 2018 18:53
djdv
djdv previously approved these changes Sep 27, 2018
@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

core/coreapi/interface/pubsub.go Show resolved Hide resolved
core/coreapi/interface/pubsub.go Show resolved Hide resolved

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)

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 magik6k force-pushed the feat/coreapi/pubsub branch from 9581396 to 0b1bd5f Compare October 2, 2018 23:42
@magik6k
Copy link
Member Author

magik6k commented Oct 2, 2018

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>
@Stebalien Stebalien force-pushed the feat/coreapi/pubsub branch from 0b1bd5f to 5618fed Compare October 5, 2018 18:51
@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
Labels
topic/core-api Topic core-api
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants