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

Retrieval Market Cleanup W/ Node Interfaces #826

Merged
merged 2 commits into from
Jan 9, 2020

Conversation

hannahhoward
Copy link
Contributor

@hannahhoward hannahhoward commented Dec 10, 2019

Goals

This PR is a WIP Branch for extracting the retrieval market to a shared component and updating it to spec.

Implementation

This extraction is based off the existing LOTUS code and V0 of the Filecoin retrieval spec: https://filecoin-project.github.io/specs/#systems__filecoin_markets__retrieval_market

The retrieval market extraction plan has a few distinct steps:

  1. Update the external interfaces of the retrieval client and provider to match those defined in spec V0
  2. Substitute direct dependencies on the LOTUS node in the retrieval client and provider with an adapter that can be implemented by either ndoe
  3. Update the internal implementation of the retrieval module to match V0 of the spec (a significant protocol change to support IPLD structures other than UnixFS + CAR file serialization)

This initial extraction does the first two steps.

This means that internal to the module, the old data types and protocols still exists (i.e. we have not done step 3) and there is translation back and forth which makes the implementation slightly awkward.

fixes filecoin-project/go-retrieval-market-project#1
fixes filecoin-project/go-retrieval-market-project#2
fixes filecoin-project/go-retrieval-market-project#3
fixes filecoin-project/go-retrieval-market-project#4

@hannahhoward hannahhoward force-pushed the feat/retrieval-market-skeleton branch 2 times, most recently from 485fa6b to 53cc90b Compare December 17, 2019 03:28
@hannahhoward hannahhoward changed the base branch from master to feat/dt-implementation December 17, 2019 09:01
@hannahhoward hannahhoward marked this pull request as ready for review December 19, 2019 23:04
@hannahhoward hannahhoward changed the title WIP: Retrieval Market Extraction Retrieval Market Cleanup W/ Node Interfaces Dec 19, 2019
@hannahhoward hannahhoward changed the base branch from feat/dt-implementation to feat/testnet2 January 8, 2020 19:30
Define all types to spec, modify interfaces, wrap old code

fix(builder): use client blockstore for retrieval

feat(retrieval): add node implementations

add node adapters for client & provider so that retrieval can be extracted
Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

1 Nit

retrieval/impl/client.go Show resolved Hide resolved
@hannahhoward
Copy link
Contributor Author

hannahhoward commented Jan 9, 2020

@magik6k lock added

Possible gotcha we should keep an eye on:
notifySubscribers calls all subscribers, which are provided from external code
if one were to block, the read lock would stay locked
(go-data-transfer has this issue as well, fwiw)

possible solutions include:

  • call each callback in go routine (downside lots of go routines)
  • copy the subscriber list and release the lock before calling callbacks (has down side of potential extra callback for someone who already unsubscribed)
  • call each callback sequentially in a go routine with a timeout

I'd recommend just deferring but worth keeping in mind

Add a mutex to protect access to the subscriber list for retrieval market
@magik6k magik6k merged commit f0f8d83 into feat/testnet2 Jan 9, 2020
@hannahhoward hannahhoward mentioned this pull request Jan 10, 2020
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.

3 participants