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

runtime: set type_url for storage Any requests #831

Merged
merged 1 commit into from
Apr 9, 2020
Merged

runtime: set type_url for storage Any requests #831

merged 1 commit into from
Apr 9, 2020

Conversation

daviddrysdale
Copy link
Contributor

The prost protobuf library does not support message descriptors, so
encapsulated Any messages from Nodes cannot fill in the type_url
field. However, the C++ protobuf library code that unpacks an Any
message into a specific protobuf message type requires this field
to be present and match.

Work around this by assuming that storage requests are of the correct
type, and modify the Any message to set its type_url field
appropriately.

Checklist

  • Pull request affects core Oak functionality (e.g. runtime, SDK, ABI)
    • I have written tests that cover the code changes.
    • I have checked that these tests are run by Cloudbuild
    • I have updated documentation accordingly.
    • I have raised an issue to
      cover any TODOs and/or unfinished work.
  • Pull request includes prototype/experimental work that is under
    construction.

The prost protobuf library does not support message descriptors, so
encapsulated Any messages from Nodes cannot fill in the type_url
field.  However, the C++ protobuf library code that unpacks an Any
message into a specific protobuf message type requires this field
to be present and match.

Work around this by assuming that storage requests are of the correct
type, and modify the Any message to set its type_url field
appropriately.
@daviddrysdale
Copy link
Contributor Author

To unblock #829

@daviddrysdale
Copy link
Contributor Author

Heh: #831 unblocks #829 which fixes #668 which unblocks #764 which will unblock #757 and #801 , the latter of which fixes #729 !

@tiziano88
Copy link
Collaborator

I was thinking to just replace that with a bytes field actually, since in general the type information is not actually coming through from gRPC, just bytes. Also strictly speaking IIRC gRPC is supposed to work with arbitrary serialization formats (though in practice I have not seen anything other than protobuf). Also it would help transform the gRPC server pseudo-node into a generic HTTP server pseudo-node, which is something I would like to be able to do (and the Rust-based version is pretty close to it already). WDYT?

@daviddrysdale
Copy link
Contributor Author

My previous feeling was that we should stick to Any, because the type indicates clearly that the contents are expected to be a serialized protobuf, and moreover it's the canonical way to encode an arbitrary protobuf message.

But the latter is less relevant if we're no longer capable of filling in the type_url part of Any, and the former could be accomplished by a comment (// contains a serialized protobuf message, whose type should be deduced from the request's method_name) rather than by using a type. So plain bytes seems reasonable now.

(Must admit I'm slightly surprised to hear you arguing for less use of a type system though!)

Copy link
Collaborator

@tiziano88 tiziano88 left a comment

Choose a reason for hiding this comment

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

Anyways let's go ahead with this to unblock things, we can follow up with the refactoring to bytes separately.

@daviddrysdale daviddrysdale merged commit 4946661 into project-oak:master Apr 9, 2020
@tiziano88
Copy link
Collaborator

FYI I am looking into the bytes refactoring.

@daviddrysdale daviddrysdale deleted the any-type branch April 9, 2020 15:04
@tiziano88 tiziano88 added this to In progress in planning via automation Apr 14, 2020
@tiziano88 tiziano88 moved this from In progress to Done in planning Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
planning
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants