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

Nr dynamic replication #56

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Nr dynamic replication #56

wants to merge 12 commits into from

Conversation

MarcPaquette
Copy link
Member

This addresses issue 52.

We've added add_replica and remove_replica to the NodeReplicated. Thread context is held within the replica . The number of new replicas are limited via MAX_REPLICAS_PER_LOG.

Implement add_replica to dynamically add replicas to a Node Replicated
data structure.
Added a remove_replica to NR.

Refactored `replicas` `lmasks` and `ltails` to use hashmaps for better
add/remove semantics.
Thread registration and deregistration remains outstanding and further
works needs to be done.
@MarcPaquette MarcPaquette marked this pull request as ready for review February 3, 2023 18:21
@gz
Copy link
Contributor

gz commented Feb 3, 2023

Had a first look, and it looks great!

I think there are some minor things to iron out:

  • I pushed a small multi-threaded example to test it, and it currently fails which is due to missing the re-routing for threads to the right replicas (in case a thread is already registered to a replica that got to be removed):
$ cargo run --example nr_dynamic_replica
   Compiling node-replication v0.2.0 (/home/gz/node-replication/node-replication)
    Finished dev [unoptimized + debuginfo] target(s) in 0.68s
     Running `target/debug/examples/nr_dynamic_replica`
About to remove replica 3
Removed replica 3
thread '<unnamed>' panicked at 'no entry found for key', /home/gz/node-replication/node-replication/src/nr/mod.rs:594:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread 'thread 'thread '<unnamed><unnamed><unnamed>' panicked at '' panicked at '' panicked at 'no entry found for keyno entry found for keyno entry found for key', ', ', /home/gz/node-replication/node-replication/src/nr/mod.rs/home/gz/node-replication/node-replication/src/nr/mod.rs/home/gz/node-replication/node-replication/src/nr/mod.rs:::681694694:::292121
  • OTOH seems like add_replicas is doing the right thing from what I can tell 🎉

  • The return value for add_replica is ThreadToken but maybe it should just be the replica id of the added replica?

  • Another thing we need to think about is that add_replica and remove_replica currently require &mut self. This is fine for now as we can just wrap it in a n RwLock for testing correctness but we probably want these to just take &self in the future.

@gz gz changed the title Nr dymanic replication Nr dynamic replication Feb 3, 2023
@MarcPaquette
Copy link
Member Author

Thanks for the comments. I'll work on addressing your feedback!

@gz gz force-pushed the nr-dymanic-replication branch from 2695e6b to c5b4ea4 Compare April 12, 2023 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants