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

Upgrade Query Protocol to Spec V0 #25

Merged
merged 8 commits into from
Jan 10, 2020
Merged

Upgrade Query Protocol to Spec V0 #25

merged 8 commits into from
Jan 10, 2020

Conversation

hannahhoward
Copy link
Collaborator

Goals

  • Use the network abstractions being worked on by @shannonwells in the retrieval implementation
  • Upgrade the Query Protocol to follow spec V0

Implementation

  • Make retrieval client/provider dependent on a network protocol implementation rather than directly on a libp2p host
  • Add a stubbable mock implementation of the retrieval network (based on @shannonwells work)
  • Update query handlers on the provider and client to use query protocol v0 from the spec
  • Comment out deal code for the time being which is not v0
  • Minor adds / changes to types
  • Add tests for retrieval client/provider for queries

shannonwells
shannonwells reviewed Jan 8, 2020
View reviewed changes
retrievalmarket/impl/client.go
@@ -188,7 +163,7 @@ type clientStream struct {
verifier BlockVerifier
bs blockstore.Blockstore
}

*/
/* This is the old retrieval code that is NOT spec compliant */
Copy link
Contributor

Choose a reason for hiding this comment

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

same re: commented out code

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do not delete in this case -- it's the whole old implementation -- need it for reference

@shannonwells shannonwells force-pushed the feat/retrieval-query-spec-v0 branch 2 times, most recently from 8bfda46 to 7c26e57 Compare January 8, 2020 23:58
hannahhoward
hannahhoward commented Jan 9, 2020
View reviewed changes
Copy link
Collaborator Author

@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.

just a reminder to resolve the master test fail before merging this (and rebase)

@hannahhoward hannahhoward mentioned this pull request Jan 9, 2020
Closed
shannonwells
shannonwells approved these changes Jan 10, 2020
View reviewed changes
Copy link
Contributor

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

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

LGTM, as soon as my flaky test PR is merged I can rebase and push up small changes (also depending on whether that other code should be deleted)

retrievalmarket/impl/provider.go
defer stream.Close()
/*
Copy link
Contributor

Choose a reason for hiding this comment

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

delete?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

no please don't for now. I'm keeping it around as a reference.

@hannahhoward hannahhoward merged commit 65eccea into master Jan 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shannonwells shannonwells shannonwells approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@hannahhoward @shannonwells