feat: new LinkMetadata iface, integrate metadata into Response type#342
feat: new LinkMetadata iface, integrate metadata into Response type#342hannahhoward merged 6 commits intorvagg/uuid-rebasingfrom
Conversation
rvagg
force-pushed
the
rvagg/metadata-2.0
branch
from
84d6385 to
e21664f
Compare
hannahhoward
left a comment
hannahhoward
left a comment
There was a problem hiding this comment.
Two general comments:
- Metadata should never be present in the extensions except when serialized to/deserialized from v1 message value. No one should be using this metadata, but in case they are, I'd rather break it
- Let's not even worry about DuplicateNotSent for this PR (much lets DuplicateTree). Let's just do this PR as the "move it out of the extensions and convert from bool to enum" step.
rvagg
force-pushed
the
rvagg/metadata-2.0
branch
from
385a862 to
ebc8c66
Compare
|
This has my latest from today, but it's broken. There's something about the metadata in the IPLD schema for v2 messages that isn't working: |
|
Pushed the fix above for the enum error :) I've also filed a bug upstream to alleviate the footgun: ipld/go-ipld-prime#348 |
…nse type * LinkMetadata wrapper around existing metadata type to allow for easier backward-compat upgrade path * integrate metadata directly into GraphSyncResponse type, moving it from an optional extension * still deal with metadata as an extension for now—further work for v2 protocol will move it into the core message schema Ref: #335
rvagg
force-pushed
the
rvagg/metadata-2.0
branch
from
2dbd766 to
13fc1ce
Compare
|
OK, so @mvdan was quick to spot the problem and pushed a fix. I found a couple more problems and did some tidy up but this now seems to be working and ready for review. For now we're just doing the two states - Present and Missing, so there's a clear mapping between old and new protocols which keeps it simple(ish). The entire metadata as extension logic is now pushed down into a purely v1 concern. The rest of the codebase assumes it's a first-class thing in the messages. |
hannahhoward
left a comment
hannahhoward
left a comment
There was a problem hiding this comment.
LGTM. I have a few nitpicks but feel free to ignore or address in a later PR.
| # been traversed entirely in the course of this request | ||
| # so I am skipping over it entirely | ||
| # | DuplicateDAGSkipped ("s") | ||
| } representation string |
There was a problem hiding this comment.
Curious -- is there a reason we went with string? Given these are super short it's probably fine, just curious.
There was a problem hiding this comment.
A bias toward descriptiveness without costing bytes. On the Go side we get to use real strings in the enum ("Present" and "Missing") to match the names here and in the message you have at least a small chance of seeing what a message contains if you capture the raw data and decode the cbor. Same bias that's applied to selector's IPLD representation, I can look at {"R":{"l":{"none":{}},":>":{"a":{">":{"@":{}}}}}} and can work out what it is if I know enough, because the symbols are suggestive and more memorable than {"0":{"1":{"none":{}},"2":{"3":{"4":{"5":{}}}}}}
There was a problem hiding this comment.
On the Go side we get to use real strings in the enum (
"Present"and"Missing") to match the names here
FYI, you could do that on bindnode with any IPLD Enum, no matter the representation strategy. It's just that for int reprs, you can also use an integer on the Go side, but you can still use a string like you do here.
Have to agree on the visual help part of the argument anyway, but wanted to clarify :)
…ng (#332) * feat(net): initial dag-cbor protocol support also added first roundtrip benchmark * feat(requestid): use uuids for requestids Ref: #278 Closes: #279 Closes: #281 * fix(requestmanager): make collect test requests with uuids sortable * fix(requestid): print requestids as string uuids in logs * fix(requestid): use string as base type for RequestId * chore(requestid): wrap requestid string in a struct * feat(libp2p): add v1.0.0 network compatibility * chore(net): resolve most cbor + uuid merge problems * feat(net): to/from ipld bindnode types, more cbor protoc improvements * feat(net): introduce 2.0.0 protocol for dag-cbor * fix(net): more bindnode dag-cbor protocol fixes Not quite working yet, still need some upstream fixes and no extensions work has been attempted yet. * chore(metadata): convert metadata to bindnode * chore(net,extensions): wire up IPLD extensions, expose as Node instead of []byte * Extensions now working with new dag-cbor network protocol * dag-cbor network protocol still not default, most tests are still exercising the existing v1 protocol * Metadata now using bindnode instead of cbor-gen * []byte for deferred extensions decoding is now replaced with datamodel.Node everywhere. Internal extensions now using some form of go-ipld-prime decode to convert them to local types (metadata using bindnode, others using direct inspection). * V1 protocol also using dag-cbor decode of extensions data and exporting the bytes - this may be a breaking change for exising extensions - need to check whether this should be done differently. Maybe a try-decode and if it fails export a wrapped Bytes Node? * fix(src): fix imports * fix(mod): clean up go.mod * fix(net): refactor message version format code to separate packages * feat(net): activate v2 network as default * fix(src): build error * chore: remove GraphSyncMessage#Loggable Ref: #332 (comment) * chore: remove intermediate v1.1 pb protocol message type v1.1.0 was introduced to start the transition to UUID RequestIDs. That change has since been combined with the switch to DAG-CBOR messaging format for a v2.0.0 protocol. Thus, this interim v1.1.0 format is no longer needed and has not been used at all in a released version of go-graphsync. Fixes: filecoin-project/lightning-planning#14 * fix: clarify comments re dag-cbor extension data As per dission in #338, we are going to be erroring on extension data that is not properly dag-cbor encoded from now on * feat: new LinkMetadata iface, integrate metadata into Response type (#342) * feat(metadata): new LinkMetadata iface, integrate metadata into Response type * LinkMetadata wrapper around existing metadata type to allow for easier backward-compat upgrade path * integrate metadata directly into GraphSyncResponse type, moving it from an optional extension * still deal with metadata as an extension for now—further work for v2 protocol will move it into the core message schema Ref: #335 * feat(metadata): move metadata to core protocol, only use extension in v1 proto * fix(metadata): bindnode expects Go enum strings to be at the type level * fix(metadata): minor fixes, tidy up naming * fix(metadata): make gofmt and staticcheck happy * fix(metadata): docs and minor tweaks after review Co-authored-by: Daniel Martí <[email protected]> * fix: avoid double-encode for extension size estimation Closes: filecoin-project/lightning-planning#15 * feat(requesttype): introduce RequestType enum to replace cancel&update bools (#352) Closes: #345 * fix(metadata): extend round-trip tests to byte representation (#350) * feat!(messagev2): tweak dag-cbor message schema (#354) * feat!(messagev2): tweak dag-cbor message schema For: 1. Efficiency: compacting the noisy structures into tuples representations and making top-level components of a message optional. 2. Migrations: providing a secondary mechanism to lean on for versioning if we want a gentler upgrade path than libp2p protocol versioning. Closes: #351 * fix(messagev2): adjust schema per feedback * feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID (#355) * feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Closes: #349 * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID when using error type T, use *T with As, rather than **T * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID * fixup! feat(graphsync): unify req & resp Pause, Unpause & Cancel by RequestID Co-authored-by: Daniel Martí <[email protected]> * feat: SendUpdates() API to send only extension data to via existing request * fix(responsemanager): send update while completing If request has finished selector traversal but is still sending blocks, I think it should be possible to send updates. As a side effect, this fixes our race. Logically, this makes sense, cause our external indicator that we're done (completed response listener) has not been called. * fix(requestmanager): revert change to pointer type * Refactor async loading for simplicity and correctness (#356) * feat(reconciledloader): first working version of reconciled loader * feat(traversalrecorder): add better recorder for traversals * feat(reconciledloader): pipe reconciled loader through code style(lint): fix static checks * Update requestmanager/reconciledloader/injest.go Co-authored-by: Rod Vagg <[email protected]> * feat(reconciledloader): respond to PR comments Co-authored-by: Rod Vagg <[email protected]> * fix(requestmanager): update test for rebase Co-authored-by: Daniel Martí <[email protected]> Co-authored-by: hannahhoward <[email protected]>
3 participants
Ref: #335
WIP, still to do:integrate into core v2 message format instead of an extensionadd aDuplicateNotSentand all the logic involved in dealing with that