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

use PieceCID if provided in QueryParams #181

Merged
merged 4 commits into from
Apr 7, 2020
Merged

Conversation

shannonwells
Copy link
Contributor

Closes #180

If QueryParams provides a PieceCID, use it and return whether the item is available.

@codecov-io
Copy link

codecov-io commented Apr 3, 2020

Codecov Report

Merging #181 into master will increase coverage by 0.01%.
The diff coverage is 85.72%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #181      +/-   ##
==========================================
+ Coverage   68.61%   68.62%   +0.01%     
==========================================
  Files          37       37              
  Lines        1991     1998       +7     
==========================================
+ Hits         1366     1371       +5     
- Misses        528      529       +1     
- Partials       97       98       +1     
Impacted Files Coverage Δ
retrievalmarket/impl/provider.go 70.52% <81.82%> (-0.15%) ⬇️
retrievalmarket/impl/client.go 67.94% <100.00%> (+0.25%) ⬆️

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 473c5d1...9a3d9f6. Read the comment docs.

}
lastErr = err
}
if lastErr == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this happens if a payloadCID was found but pieceCID wasn't

@@ -277,7 +285,7 @@ func (p *provider) NextBlock(ctx context.Context, id retrievalmarket.ProviderDea
}

func (p *provider) GetPieceSize(c cid.Cid) (uint64, error) {
pieceInfo, err := getPieceInfoFromCid(p.pieceStore, c)
pieceInfo, err := getPieceInfoFromCid(p.pieceStore, c, cid.Undef)
Copy link
Contributor Author

@shannonwells shannonwells Apr 3, 2020

Choose a reason for hiding this comment

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

This is called by provider_states.ReceiveDeal, and DealProposal doesn't know anything about individual PieceCIDs. I'm assuming that client proposing a deal will always do a Query with the PieceCID first so unless PieceCID is added to DealProposal, we assume client has already determined the provider has what they want.

@shannonwells shannonwells marked this pull request as ready for review April 3, 2020 23:12
Status QueryResponseStatus
//PayloadCIDFound QueryItemStatus // V1 - if a PayloadCid was requested, the result
Status QueryResponseStatus
PieceCIDFound QueryItemStatus // V1 - if a PieceCID was requested, the result
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seemed to be a typo/leftover from the old version when Piece/Payload CID weren't distinct

Copy link
Collaborator

Choose a reason for hiding this comment

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

originally PayloadCIDs were optional while PieceCIDs weren't. But this is still correct :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, right that was it.

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 regenerate CBOR for the project, then fix the failing tests that appear when it is.

I think you will find that PieceCID will have to be a pointer cause cid.Undef does not serialize with cbor-gen.

retrievalmarket/impl/provider.go Outdated Show resolved Hide resolved
retrievalmarket/impl/provider.go Outdated Show resolved Hide resolved
Status QueryResponseStatus
//PayloadCIDFound QueryItemStatus // V1 - if a PayloadCid was requested, the result
Status QueryResponseStatus
PieceCIDFound QueryItemStatus // V1 - if a PieceCID was requested, the result
Copy link
Collaborator

Choose a reason for hiding this comment

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

originally PayloadCIDs were optional while PieceCIDs weren't. But this is still correct :)

@hannahhoward hannahhoward merged commit d099de3 into master Apr 7, 2020
@hannahhoward hannahhoward deleted the feat/query-pieceCID branch April 30, 2020 21:33
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.

Retrieval Query's support specifying optional PieceCID
3 participants