Sim2h - RRDHT "R-Value" Stub Calculation #1798
Conversation
neonphog
added
NEEDS REVIEW
crates/sim2h/src/lib.rs
Outdated
| num_ticks: u32, | ||
| num_ticks: u64, | ||
| /// sim2h currently uses the same radius for all connections | ||
| rrdht_arc_radius: u32, |
There was a problem hiding this comment.
Hm, I think this is the wrong place for this variable to be. The sim2h server can server many spaces, and each space will have a different aggregated arc radius, no?
crates/sim2h/src/lib.rs
Outdated
| for (_k, c_state) in self.connection_states.read().iter() { | ||
| match c_state { | ||
| ConnectionState::Limbo(_) => (), | ||
| ConnectionState::Joined(_, _, dht_data) => { |
There was a problem hiding this comment.
Here we are neglecting the space that agent is in and assuming all agents to be in the space, right? I think we should calculate r-values per space.
There was a problem hiding this comment.
I think lucksus is right. Also, we probably what to do something other than recalc every 20seconds i.e. rather much less recalc after significant changes, but that's fine for a followup. Maybe just change to a function should_recalc() to encapsulate the semantics?
There was a problem hiding this comment.
Cool, looks good!
I wonder if we should add tests on this level too (=unit testing sim2h) as this code does not result in a change of behavior and thus can't be picked up by our current set of tests. But totally ok to merge this as it is and add tests with the next PR that actually applies the sharding.
Note that we most likely won't see any effect of that in our current app-spec as we only have up to 3 nodes there. The new stress tests can be run with any N, but for performance and cost reasons, CI is set to 10. So in order to test rrDHT sharding I think it would be good to cover a good part of it with unit tests here - and then also add specific stress tests where the number of nodes and the target R-value are set in a way that the sharding code actually gets hit.
PR summary
Adds rrdht "R-Value" calculation for storage arc radius to sim2h in a stub manner
testing/benchmarking notes
( if any manual testing or benchmarking was/should be done, add notes and/or screenshots here )
followups
( any new tickets/concerns that were discovered or created during this work but aren't in scope for review here )
changelog
Please check one of the following, relating to the CHANGELOG-UNRELEASED.md
- summary of change [PR#1234](https://github.com/holochain/holochain-rust/pull/1234)