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

Move gRPC to core #7

Merged
merged 5 commits into from Nov 30, 2021
Merged

Move gRPC to core #7

merged 5 commits into from Nov 30, 2021

Conversation

penberg
Copy link
Contributor

@penberg penberg commented Nov 29, 2021

As suggested by @glommer, let's move gRPC to core to make it easier for
users to embed ChiselStore out-of-the-box. Users can, of course, also
implement the necessary traits themselves if they don't want to use
gRPC.

@penberg penberg requested a review from glommer November 29, 2021 13:12
@penberg penberg marked this pull request as ready for review November 29, 2021 13:12
Copy link

@glommer glommer left a comment

Choose a reason for hiding this comment

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

Pekka, please add a variation of #![warn(missing_docs, missing_debug_implementations, rust_2018_idioms)] to the top-level lib file so we make sure that all public interfaces have Debug, documentation, etc (likely you will want to enforce 2021 idioms)

#[tokio::main]
async fn main() -> Result<()> {
let opt = Opt::from_args();
let port = 50000 + opt.id;
let addr = format!("127.0.0.1:{}", port).parse().unwrap();
let transport = Transport {};
let transport = RpcTransport::new(Box::new(node_addr));
Copy link

Choose a reason for hiding this comment

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

I personally think it is better to have a single interface, Transport, that can take a trait so the user would write
Transport<RpcTransport>. This way we can have more code sharing between transports, and even allow the user to extend their transport more easily. For example, I want another RPC transport, but I want a specific verb to behave a bit differently

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you give me an example of what that single interface would look like? is Transport a struct?

@penberg penberg merged commit a2f2e2b into main Nov 30, 2021
@penberg penberg deleted the penberg/move-grpc-to-core branch November 30, 2021 07:40
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.

None yet

2 participants