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

Support multiple metadata transport protocols #198

Merged
merged 2 commits into from
Mar 16, 2022
Merged

Conversation

masih
Copy link
Member

@masih masih commented Mar 15, 2022

Introduce the Transport type to capture information about multiple
transport protocols within a single metadata.

Re implement Metadata type to accommodate:

  • multiple transport protocols
  • enforce they are sorted by their ID
  • backward compatibility with existing metadata format
  • deserialization of existing formats gracefully.

Add metadata conversion to keep code compiling until this PR is merged:
- ipni/storetheindex#250

The PR now depends on the head of sti main now that this PR is merged.

Fixes: #188

@masih
Copy link
Member Author

masih commented Mar 15, 2022

@gammazero I think this PR would make #196 redundant if it only contains multi metadata support

@codecov-commenter
Copy link

codecov-commenter commented Mar 15, 2022

Codecov Report

Merging #198 (bf3ee40) into main (2852cc9) will increase coverage by 0.97%.
The diff coverage is 65.86%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #198      +/-   ##
==========================================
+ Coverage   38.17%   39.15%   +0.97%     
==========================================
  Files          49       53       +4     
  Lines        3526     3678     +152     
==========================================
+ Hits         1346     1440      +94     
- Misses       1929     1973      +44     
- Partials      251      265      +14     
Impacted Files Coverage Δ
cmd/provider/index.go 0.00% <0.00%> (ø)
server/admin/http/importcar_handler.go 71.42% <25.00%> (-11.43%) ⬇️
cmd/provider/import.go 55.55% <41.66%> (-3.71%) ⬇️
metadata/bitswap.go 62.50% <62.50%> (ø)
engine/engine.go 60.58% <66.66%> (-0.73%) ⬇️
metadata/metadata.go 73.68% <73.68%> (ø)
cardatatransfer/cardatatransfer.go 61.97% <75.00%> (-0.27%) ⬇️
metadata/graphsync_filecoinv1.go 81.63% <78.57%> (-4.74%) ⬇️
metadata/hack.go 78.57% <78.57%> (ø)
metadata/counting_reader.go 100.00% <100.00%> (ø)
... and 3 more

engine/engine_test.go Outdated Show resolved Hide resolved
metadata/graphsync_filecoinv1.go Outdated Show resolved Hide resolved
metadata/metadata.go Outdated Show resolved Hide resolved
metadata/metadata.go Outdated Show resolved Hide resolved
Introduce the `Transport` type to capture information about multiple
transport protocols within a single metadata.

Re implement `Metadata` type to accommodate:
* multiple transport protocols
* enforce they are sorted by their ID
* backward compatibility with existing metadata format
* deserialization of existing formats gracefully.

Add metadata conversion to keep code compiling until this PR is merged:
 - ipni/storetheindex#250

Fixes: #188
masih added a commit to ipni/storetheindex that referenced this pull request Mar 15, 2022
Remove the `v0.Metadata` type in favour of a simpler `[]byte` in
preparation for moving all metadata types entirely to index provider.

From this point onwards, metadata is treated as an opaque payload by the
indexer. Users should use index-provider library to decode it instead.

Relates to:
- ipni/index-provider#198
@masih masih marked this pull request as draft March 15, 2022 21:21
@masih
Copy link
Member Author

masih commented Mar 15, 2022

Marking as draft until this is merged: ipni/storetheindex#260

masih added a commit to ipni/storetheindex that referenced this pull request Mar 16, 2022
Remove the `v0.Metadata` type in favour of a simpler `[]byte` in
preparation for moving all metadata types entirely to index provider.

From this point onwards, metadata is treated as an opaque payload by the
indexer. Users should use index-provider library to decode it instead.

Relates to:
- ipni/index-provider#198
@masih masih marked this pull request as ready for review March 16, 2022 09:15
Copy link
Collaborator

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

Awesome! See comments.

Usage: "Specify retrieval protocol ID",
Required: true,
},
metadataFlag,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is fine for now, but we may want to have a graphsync and a bitswap flag to allow the user to specify the metadata protocol and contents.

Copy link
Member Author

Choose a reason for hiding this comment

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

Captured: #199

metadata/metadata.go Outdated Show resolved Hide resolved
Depend on head of PR that removes `Metadata` type in storetheindex so
that we can remove all workarounds:
- ipni/storetheindex#260
@masih masih merged commit cfaa2a3 into main Mar 16, 2022
@masih masih deleted the masih/metadata-api-mv branch March 16, 2022 17:42
masih added a commit to ipni/storetheindex that referenced this pull request Mar 16, 2022
This reverts commit 537eb37 now that
the index-provider is upgraded.

See:
 - ipni/index-provider#198
masih added a commit to ipni/storetheindex that referenced this pull request Mar 16, 2022
This reverts commit 537eb37 now that
the index-provider is upgraded.

See:
 - ipni/index-provider#198
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.

Multiple protocol support in metadata
3 participants