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

Replace the UDP replication protocol with gRPC for the distributed store #337

Merged
merged 2 commits into from
May 22, 2024

Conversation

chirino
Copy link
Contributor

@chirino chirino commented May 17, 2024

No description provided.

@chirino chirino force-pushed the distributed-grpc branch from 0b557ea to 1587550 Compare May 21, 2024 11:36
@chirino
Copy link
Contributor Author

chirino commented May 21, 2024

rebased.

@chirino chirino requested a review from alexsnaps May 21, 2024 12:02
Copy link
Member

@alexsnaps alexsnaps left a comment

Choose a reason for hiding this comment

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

Approving, but thinking we should split the mod.rs up, as it's getting big and still these types still lack some unit tests...
Left a few other comments, we'll iterate as we go!
Thanks 🙏

},
Some(Err(err)) => {
if is_disconnect(&err) {
println!("peer: '{}': disconnected: {:?}", self.peer_id, err);
Copy link
Member

Choose a reason for hiding this comment

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

These could become tracing::{error!, warn!, info!} macros... Same syntax. Error level is enabled by default when launching the server, while warn, info, debug, trace are -v, -vv, -vvv or -vvvv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.


async fn process(&mut self, in_stream: &mut Streaming<Packet>) -> Result<(), Status> {
// Send a MembershipUpdate to inform the peer about all the members
// We should resend it again if we learn of new members.
Copy link
Member

Choose a reason for hiding this comment

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

So the intent is to have every node share the nodes it knows about? Is there a way to "forget about a node"? Or is the idea to keep the set a grow only set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need to think about this one a bit more.. maybe they should have some kinda of lease that gets renewed? not sure.

use std::path::Path;

fn main() -> Result<(), Box<dyn Error>> {
generate_protobuf()
Copy link
Member

Choose a reason for hiding this comment

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

This probably should be conditional on the feature being enabled... see cfg!, such as here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@chirino chirino merged commit b89185d into Kuadrant:main May 22, 2024
8 checks passed
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.

2 participants