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

fix(kad): close progress whenever closer in range #4934

Merged
merged 1 commit into from Apr 3, 2024

Conversation

maqi
Copy link
Contributor

@maqi maqi commented Nov 27, 2023

Closer shall be calculated as closer to the seen num_results peers,
not just to be being the closest as current.

This is in parallel to the PR #4932
Split them into two to make the work easier to be discussed and reviewed.

Description

Notes & open questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

@maqi maqi force-pushed the close_progress_whenever_got_closer branch 2 times, most recently from 2b333db to 9324fa1 Compare November 27, 2023 15:13
@maqi maqi force-pushed the close_progress_whenever_got_closer branch 2 times, most recently from 525faad to 3865016 Compare November 28, 2023 10:12
Copy link

mergify bot commented Dec 1, 2023

This pull request has merge conflicts. Could you please resolve them @maqi? 🙏

@maqi maqi force-pushed the close_progress_whenever_got_closer branch from 3865016 to 5257090 Compare December 5, 2023 13:08
@maqi
Copy link
Contributor Author

maqi commented Dec 5, 2023

This pull request has merge conflicts. Could you please resolve them @maqi? 🙏

PR is updated, sorry for the delay

@maqi
Copy link
Contributor Author

maqi commented Dec 5, 2023

Hi, @mxinden ,

here is the follow up fix in parallel to the PR #4932

let me know if it make sense.
thx

Copy link

mergify bot commented Feb 19, 2024

This pull request has merge conflicts. Could you please resolve them @maqi? 🙏

@guillaumemichel guillaumemichel self-requested a review March 14, 2024 15:40
Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

Thanks @maqi for looking into this, it is an important issue!

protocols/kad/src/query/peers/closest.rs Outdated Show resolved Hide resolved
protocols/kad/src/query/peers/closest.rs Outdated Show resolved Hide resolved
@maqi maqi force-pushed the close_progress_whenever_got_closer branch from 5257090 to bd15207 Compare April 3, 2024 06:14
@maqi
Copy link
Contributor Author

maqi commented Apr 3, 2024

Hi, @guillaumemichel ,

Really appreciate your comments and suggestions.

Sorry for the late reply, have not check back this crate for a while as had been a while not receive any feedback.

Thx again and hope this PR could be merged soon.

Best Regards,

@maqi
Copy link
Contributor Author

maqi commented Apr 3, 2024

The failing clippy as at https://github.com/libp2p/rust-libp2p/actions/runs/8534098889/job/23377901550?pr=4934
seems not related to this PR ?

Copy link
Contributor

@guillaumemichel guillaumemichel left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks for your contribution @maqi!

@mergify mergify bot merged commit 3d16353 into libp2p:master Apr 3, 2024
71 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants