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

Storagemarket/provider allows subscription to events #202

Merged
merged 3 commits into from Apr 24, 2020

Conversation

shannonwells
Copy link
Contributor

@shannonwells shannonwells commented Apr 23, 2020

WAT

closes #77

HAO

  • Allow consumers to subscribe to StorageProvider events.
  • Use @hannahhoward new generic threadsafe pubsub for subscription management

Questions

I'm sending the entire deal to the subscriber. Not sure if that's what we really want though.


// -------
// providerDealEnvironment
// -------
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I kept paging too far down looking at stuff, so I figured a comment might help

@codecov-io
Copy link

codecov-io commented Apr 23, 2020

Codecov Report

Merging #202 into master will decrease coverage by 0.74%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #202      +/-   ##
==========================================
- Coverage   67.88%   67.15%   -0.73%     
==========================================
  Files          40       40              
  Lines        2117     2161      +44     
==========================================
+ Hits         1437     1451      +14     
- Misses        573      601      +28     
- Partials      107      109       +2     
Impacted Files Coverage Δ
storagemarket/impl/client.go 0.00% <ø> (ø)
storagemarket/impl/provider.go 6.20% <0.00%> (-0.79%) ⬇️
storagemarket/types.go 33.34% <ø> (ø)
retrievalmarket/network/deal_stream.go 65.39% <0.00%> (+15.39%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb08236...fe99358. Read the comment docs.

@shannonwells shannonwells marked this pull request as ready for review April 23, 2020 23:39
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

Please remove go routine and fix test
Convert providerSubscriber to take a MinerDeal value, not a reference. Passing the whole deal is correct but should not be a pointer.

storagemarket/impl/client.go Outdated Show resolved Hide resolved
storagemarket/types.go Outdated Show resolved Hide resolved
storagemarket/impl/provider.go Outdated Show resolved Hide resolved
storagemarket/impl/provider.go Outdated Show resolved Hide resolved
@shannonwells shannonwells force-pushed the feat/storagemarket-provider-subscriptions-#77 branch from e128c68 to fe99358 Compare April 24, 2020 17:38
Copy link
Collaborator

@hannahhoward hannahhoward left a comment

Choose a reason for hiding this comment

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

LGTM and that's a really good test for state flow.

}
}

expectedStates := []storagemarket.StorageDealStatus{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Love this check!

@shannonwells shannonwells merged commit 7515137 into master Apr 24, 2020
@shannonwells shannonwells deleted the feat/storagemarket-provider-subscriptions-#77 branch April 24, 2020 18:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User of storage provider can listen for events on deals
3 participants