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.

Sign up for GitHub

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

Jump to bottom

fix(kad): compute jobs_query_capacity accurately #5148

Merged
merged 6 commits into from
Mar 27, 2024
Merged

fix(kad): compute jobs_query_capacity accurately #5148

merged 6 commits into from
Mar 27, 2024

Conversation

junekhan
Copy link
Contributor

@junekhan junekhan commented Feb 5, 2024 •
edited by thomaseizinger
Loading

Description

Fixes: #4191.

Notes & open questions

@junekhan junekhan changed the title Compute accurate jobs_query_capacity fix: compute accurate jobs_query_capacity Feb 6, 2024
@thomaseizinger thomaseizinger changed the title fix: compute accurate jobs_query_capacity fix(kad): compute accurate jobs_query_capacity Feb 12, 2024
@thomaseizinger thomaseizinger changed the title fix(kad): compute accurate jobs_query_capacity fix(kad): compute jobs_query_capacity accurately Feb 12, 2024
thomaseizinger
thomaseizinger reviewed Feb 12, 2024
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thank you!

Can we add a test please to ensure we don't break this in the future?

@junekhan
Copy link
Contributor Author

Thank you!

Can we add a test please to ensure we don't break this in the future?

@thomaseizinger Sure thing! But frankly speaking, I'm a novice in Rust, and it may take a good amount of time to finalize the test case. And, can I ask you for a favor to show me the best test case that I can refer to in this file? I feel it can somehow speed up us a little bit. Thanks!

@thomaseizinger
Copy link
Contributor

Thank you!
Can we add a test please to ensure we don't break this in the future?

@thomaseizinger Sure thing! But frankly speaking, I'm a novice in Rust, and it may take a good amount of time to finalize the test case. And, can I ask you for a favor to show me the best test case that I can refer to in this file? I feel it can somehow speed up us a little bit. Thanks!

Unfortunately, this area of the code is also pretty foreign to me and I don't have the bandwidth to dig into it deeply.

thomaseizinger
thomaseizinger previously approved these changes Feb 14, 2024
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Cool! Happy to merge this without a test now.

Can you bump the version of the crate and add a changelog entry? See docs for details :)

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024

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

@junekhan
Merge remote-tracking branch 'upstream/master' into fix/compute-accur…
64f6c70 
…ate-jobs_query_capacity

# Conflicts:
#	protocols/kad/CHANGELOG.md
@junekhan junekhan force-pushed the fix/compute-accurate-jobs_query_capacity branch from 321c4bc to 52d88d3 Compare February 19, 2024 07:49
jxs
jxs previously approved these changes Mar 14, 2024
View reviewed changes
Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

Thanks June for this, and Thomas for reviewing it! LGTM cc @iand

@jxs jxs added the send-it label Mar 14, 2024
@mergify mergify bot dismissed stale reviews from thomaseizinger and jxs March 27, 2024 17:38

Approvals have been dismissed because the PR was updated after the send-it label was applied.

@mergify mergify bot merged commit 89c684a into libp2p:master Mar 27, 2024
71 of 72 checks passed
guillaumemichel pushed a commit that referenced this pull request Mar 28, 2024
@junekhan @guillaumemichel
fix(kad): compute jobs_query_capacity accurately
e5346ec 
Fixes: #4191.

Pull-Request: #5148.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger left review comments

@jxs jxs jxs approved these changes

Assignees
No one assigned
Labels
send-it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

protocols/kad: accounting error for background job capacity
3 participants
@junekhan @thomaseizinger @jxs