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

Add RPC v2 support #2544

Draft
wants to merge 57 commits into
base: main
Choose a base branch
from
Draft

Add RPC v2 support #2544

wants to merge 57 commits into from

Conversation

david-perez
Copy link
Contributor

@david-perez david-perez commented Apr 4, 2023

RPC v2 is a new protocol that is being added has recently been added to the Smithy
specification.

(I'll add more details here as the patchset evolves)

Credit goes to @jjant for initial implementation of the router, which I built on top of from his jjant/smithy-rpc-v2-exploration branch.

Caveats

TODOs are currently exhaustively sprinkled throughout the patch documenting what remains to be done. Most of these need to be addressed before this can be merged in; some can be punted on to not make this PR bigger.

However, I'd like to call out the major caveats and blockers here. I'll keep updating this list as the patchset evolves.

  • RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the smithy-rpc-v2 branch. The following are prerrequisites for this PR to be merged; until they are done CI on this PR will fail:
  • Protocol tests for the protocol do not currently exist in Smithy. Until those get written, this PR resorts to Rust unit tests and integration tests that use serde to round-trip messages and compare serde's encoders and decoders with ours for correctness.
  • Currently only server-side support has been implemented, because that's what I'm most familiar. However, we're almost all the way there to add client-side support, so we may as well do it in this PR too.
  • [ ] Smithy document shapes are not supported. RPC v2's specification currently doesn't indicate how to implement them.
    • The spec ended up leaving them as unsupported: "Document types are not currently supported in this protocol."

Prerequisite PRs

This section lists prerequisite PRs and issues that would make the diff for this one lighter or easier to understand. It's preferable that these PRs be merged prior to this one; some are hard prerequisites. They mostly relate to parts of the codebase I've had to touch or pilfer inspect in this PR where I've made necessary changes, refactors and "drive-by improvements" that are mostly unrelated, although some directly unlock things I've needed in this patchset. It makes sense to pull them out to ease reviewability and make this patch more semantically self-contained.

Testing

RPC v2 has still not been added to the Smithy specification. It is currently being worked on over in the
smithy-rpc-v2 branch.

This can only be tested locally following these steps:

1. Clone the Smithy repository and checkout the smithy-rpc-v2 branch.
2. Inside your local checkout of smithy-rs pointing to this PR's branch, make sure you've added mavenLocal() as a repository in the build.gradle.kts files. Example.
4. Inside your local checkout of Smithy's smithy-rpc-v2 branch:
1. Set VERSION to the current Smithy version used in smithy-rs (1.28.1 as of writing, but check here).
2. Run ./gradlew clean build pTML.

6. 1. In your local checkout of the smithy-rs's smithy-rpc-v2 branch, run ./gradlew codegen-server-test:build -P modules='rpcv2'.

You can troubleshoot whether you have Smithy correctly set up locally by inspecting ~/.m2/repository/software/amazon/smithy/smithy-protocols-traits.

Checklist

  • I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

RPC v2 is a new protocol that is being added to the Smithy
specification.
… 9cde04f31a96a37b49c4f88a91d87aae44efecbd`, compile
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant