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

Dht diff #59

Merged
merged 28 commits into from
Jan 3, 2025
Merged

Dht diff #59

merged 28 commits into from
Jan 3, 2025

Conversation

ThetaSinner
Copy link
Member

@ThetaSinner ThetaSinner commented Dec 18, 2024

Top level DHT model and diff comparison to figure out which ops might be missing from the other.

I know there's a lot of code addition on this PR. I think the best place to start is probably the module documentation, then following that into the entry functions like snapshot_minimal. That should be good for a high-level understanding I hope. Then the test harness crates/dht/tests/harness.rs is a good place to understand how the sync is done. That code will be more or less what gossip needs to do to use the code under test.

If it helps, there is +1574 just from tests. So only half of this is actually new production code.

Following this PR I think there's a need to go back over the code in this crate and ensure consistent naming and documentation. I think the new documentation is consistent but I don't think it's entirely consistent with what's in here from previous PRs.

@ThetaSinner ThetaSinner requested a review from a team December 19, 2024 13:28
@ThetaSinner ThetaSinner marked this pull request as ready for review December 19, 2024 13:29
@ThetaSinner ThetaSinner mentioned this pull request Dec 19, 2024
neonphog
neonphog previously approved these changes Dec 19, 2024
Copy link
Collaborator

@neonphog neonphog left a comment

Choose a reason for hiding this comment

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

Couple questions including the potential name change, but the code here looks good. Nice work 👍

Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

More tomorrow

crates/dht/src/time.rs Outdated Show resolved Hide resolved
crates/dht/src/combine.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Great looking code! I made some suggestions.

crates/dht/src/hash.rs Outdated Show resolved Hide resolved
crates/dht/src/dht.rs Show resolved Hide resolved
crates/dht/src/dht.rs Show resolved Hide resolved
crates/dht/src/dht.rs Outdated Show resolved Hide resolved
crates/dht/src/dht/snapshot.rs Show resolved Hide resolved
crates/dht/src/dht/snapshot.rs Outdated Show resolved Hide resolved
crates/dht/src/dht/tests.rs Show resolved Hide resolved
crates/dht/src/dht/tests.rs Show resolved Hide resolved
crates/dht/src/dht/tests.rs Show resolved Hide resolved
crates/dht/src/dht/snapshot.rs Show resolved Hide resolved
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Some more comments

crates/dht/src/arc_set.rs Show resolved Hide resolved
crates/dht/src/dht.rs Show resolved Hide resolved
crates/dht/src/dht/tests.rs Show resolved Hide resolved
Copy link
Contributor

@jost-s jost-s left a comment

Choose a reason for hiding this comment

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

Very happy with this code!

@ThetaSinner ThetaSinner merged commit 448c23a into main Jan 3, 2025
5 checks passed
@ThetaSinner ThetaSinner deleted the dht-diff branch January 3, 2025 15:42
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.

5 participants