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

Implements BalancedPool #1064

Merged
merged 11 commits into from
Oct 18, 2021
Merged

Implements BalancedPool #1064

merged 11 commits into from
Oct 18, 2021

Conversation

mcollina
Copy link
Member

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Oct 16, 2021 •
edited
Loading

Codecov Report

Merging #1064 (1aa8dd1) into main (e402e2e) will increase coverage by 0.00%.
The diff coverage is 95.16%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1064   +/-   ##
=======================================
  Coverage   94.92%   94.93%           
=======================================
  Files          37       38    +1     
  Lines        3647     3709   +62     
=======================================
+ Hits         3462     3521   +59     
- Misses        185      188    +3
Impacted Files Coverage Δ
lib/balanced-pool.js 94.54% <94.54%> (ø)
index.js 100.00% <100.00%> (ø)
lib/core/errors.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e402e2e...1aa8dd1. Read the comment docs.

ronag
ronag reviewed Oct 16, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Oct 16, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Oct 16, 2021
View reviewed changes
lib/balanced_pool.js Outdated
const pool = this[kPools].find((pool) => pool.upstream === upstream)
const idx = this[kPools].indexOf(pool)
this[kPools].splice(idx, 1)
pool.close()
Copy link
Member

Choose a reason for hiding this comment

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

there is a race here I believe. If you remove upstream the event handlers are still there and the pool is still closing. What if upstream is re-added added before finishing closing? Can be a little confusing behavior.

ronag
ronag requested changes Oct 16, 2021
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

I think destroy and close need to return promsie if not passed callback.

@mcollina
Copy link
Member Author

I think destroy and close need to return promsie if not passed callback.

They already do. What's missing specifically?

@ronag
Copy link
Member

ronag commented Oct 17, 2021

I think destroy and close need to return promsie if not passed callback.

They already do. What's missing specifically?

You don’t return anything?

mcollina
mcollina commented Oct 17, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag approved these changes Oct 17, 2021
View reviewed changes
Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

LGTM.

I still think there is a race here with removing/adding upstreams but it's kind of an unlikely edge case.

There is room for improvement in regards to the algorithm but this should be good enough to be useful.

ronag
ronag reviewed Oct 17, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Oct 17, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
@mcollina
Copy link
Member Author

There is room for improvement in regards to the algorithm but this should be good enough to be useful.

I'll open an issue afterwards, the selection algorithm can be improved significantly and it will be a good contribution!

ronag
ronag reviewed Oct 18, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
@mcollina @ronag
Update lib/balanced_pool.js
9919df9 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
ronag
ronag reviewed Oct 18, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Oct 18, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
ronag
ronag reviewed Oct 18, 2021
View reviewed changes
lib/balanced_pool.js Outdated Show resolved Hide resolved
mcollina and others added 3 commits October 18, 2021 09:37
@mcollina @ronag
Apply suggestions from code review
1c528e9 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag
fixup
88fe40c
@ronag
fixup
e674bb3
ronag
ronag reviewed Oct 18, 2021
View reviewed changes
index.js Show resolved Hide resolved
@mcollina
Copy link
Member Author

Updated things a bit, tests are passing

ronag
ronag reviewed Oct 18, 2021
View reviewed changes
lib/balanced-pool.js Outdated Show resolved Hide resolved
@mcollina @ronag
Update lib/balanced-pool.js
1aa8dd1 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag ronag merged commit 5647c93 into main Oct 18, 2021
@mcollina mcollina deleted the balanced-pool branch October 18, 2021 13:34
KhafraDev pushed a commit to KhafraDev/undici that referenced this pull request Jun 23, 2022
@mcollina @ronag @KhafraDev
Implements BalancedPool (nodejs#1064)
9d590da 
* Implements BalancedPool

* added new doc page to sidebar

* Review comments

* removed todos

* Update lib/balanced_pool.js

Co-authored-by: Robert Nagy <ronagy@icloud.com>

* Apply suggestions from code review

Co-authored-by: Robert Nagy <ronagy@icloud.com>

* fixup

* fixup

* fixup

* Fixed load balancing

* Update lib/balanced-pool.js

Co-authored-by: Robert Nagy <ronagy@icloud.com>

Co-authored-by: Robert Nagy <ronagy@icloud.com>
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
@mcollina @ronag @crysmags
Implements BalancedPool (nodejs#1064)
24ece97 
* Implements BalancedPool

* added new doc page to sidebar

* Review comments

* removed todos

* Update lib/balanced_pool.js

Co-authored-by: Robert Nagy <ronagy@icloud.com>

* Apply suggestions from code review

Co-authored-by: Robert Nagy <ronagy@icloud.com>

* fixup

* fixup

* fixup

* Fixed load balancing

* Update lib/balanced-pool.js

Co-authored-by: Robert Nagy <ronagy@icloud.com>

Co-authored-by: Robert Nagy <ronagy@icloud.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ronag ronag ronag approved these changes

@delvedor delvedor Awaiting requested review from delvedor

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mcollina @codecov-commenter @ronag