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

Resizable semaphore #1019

Merged
merged 14 commits into from
Dec 27, 2022
Merged

Resizable semaphore #1019

merged 14 commits into from
Dec 27, 2022

Conversation

rahulksnv
Copy link
Contributor

@rahulksnv rahulksnv commented Dec 13, 2022

This is a draft PR based off Nazar's #1017. Specifically this discussion thread: #1017 (comment)

Main change:

  1. Implements a semaphore style construct(ResizableSemaphore) that allows both expanding and shrinking of the max permits
  2. This uses a constant space/time
  3. When the max permits are shrinked: new permit requests would block until outstanding permits are freed and the usage falls below the new lower capacity
  4. Unit tests for ResizableSemaphore

End to end testing: TODO

Code contributor checklist:

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

Generally I like the API, it is much nicer to use and doesn't require timer, which is even better!

I have some major comments as well as a bunch of nits about our contributing practices and idiomatic Rust more broadly.


We don't need to see every single step as you were iterating on the implementation in Git history, squash things before creating PR and look through it to see if unnecessary empty lines or other small nuances were accidentally committed, see CONTRIBUTING.md, which talks about that and much more.
Commits like Clean up should certainly be squashed with original commit it cleans up before pushing, also the whole thing needs to be rebased so that there is no avoidable merge commit in the middle of the history right away.
Please group data structures and corresponding impls together instead of interleaving various definitions with various impls.
Ideally, though not always possible/convenient, data structures should be defined before they are used.

Also latest test run failed and rebase will be necessary due to changes in the branch this targets.

UPD: "Code contributor checklist:" is there to be followed and checked before submitting a PR!

@rahulksnv
Copy link
Contributor Author

Generally I like the API, it is much nicer to use and doesn't require timer, which is even better!

I have some major comments as well as a bunch of nits about our contributing practices and idiomatic Rust more broadly.

We don't need to see every single step as you were iterating on the implementation in Git history, squash things before creating PR and look through it to see if unnecessary empty lines or other small nuances were accidentally committed, see CONTRIBUTING.md, which talks about that and much more. Commits like Clean up should certainly be squashed with original commit it cleans up before pushing, also the whole thing needs to be rebased so that there is no avoidable merge commit in the middle of the history right away. Please group data structures and corresponding impls together instead of interleaving various definitions with various impls. Ideally, though not always possible/convenient, data structures should be defined before they are used.

Also latest test run failed and rebase will be necessary due to changes in the branch this targets.

UPD: "Code contributor checklist:" is there to be followed and checked before submitting a PR!

Thanks for the feedback. I will address the comments and update you

@rahulksnv
Copy link
Contributor Author

Generally I like the API, it is much nicer to use and doesn't require timer, which is even better!
I have some major comments as well as a bunch of nits about our contributing practices and idiomatic Rust more broadly.
We don't need to see every single step as you were iterating on the implementation in Git history, squash things before creating PR and look through it to see if unnecessary empty lines or other small nuances were accidentally committed, see CONTRIBUTING.md, which talks about that and much more. Commits like Clean up should certainly be squashed with original commit it cleans up before pushing, also the whole thing needs to be rebased so that there is no avoidable merge commit in the middle of the history right away. Please group data structures and corresponding impls together instead of interleaving various definitions with various impls. Ideally, though not always possible/convenient, data structures should be defined before they are used.
Also latest test run failed and rebase will be necessary due to changes in the branch this targets.
UPD: "Code contributor checklist:" is there to be followed and checked before submitting a PR!

Thanks for the feedback. I will address the comments and update you

I will squash the commits next

Copy link
Member

@nazar-pc nazar-pc 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, just a few minor things

Copy link
Member

@nazar-pc nazar-pc left a comment

Choose a reason for hiding this comment

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

One nit, looks great otherwise!

@nazar-pc nazar-pc marked this pull request as ready for review December 27, 2022 18:23
@nazar-pc
Copy link
Member

Will merge once CI passes, thanks!

@rahulksnv rahulksnv merged commit 6e989d3 into libp2p-stream-limits Dec 27, 2022
@rahulksnv rahulksnv deleted the rsub/libp2p-stream-limits branch December 27, 2022 18:34
@nazar-pc
Copy link
Member

Hm... I was actually waiting for CI to pass before merging

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.

2 participants