feat: use weighted round robin in balancedPool #1069

jodevsa · 2021-10-20T20:31:16Z

lib/balanced-pool.js

codecov-commenter · 2021-10-20T20:33:55Z

Codecov Report

Merging #1069 (467f847) into main (5ca25c2) will increase coverage by 1.47%.
The diff coverage is 97.91%.

❗ Current head 467f847 differs from pull request most recent head 1b4bc6b. Consider uploading reports for the commit 1b4bc6b to get more accurate results

@@            Coverage Diff             @@
##             main    #1069      +/-   ##
==========================================
+ Coverage   94.90%   96.37%   +1.47%     
==========================================
  Files          50       37      -13     
  Lines        4608     2785    -1823     
==========================================
- Hits         4373     2684    -1689     
+ Misses        235      101     -134

Impacted Files	Coverage Δ
lib/balanced-pool.js	`98.83% <97.91%> (-1.17%)`	⬇️
index.js	`84.28% <0.00%> (-15.72%)`	⬇️
lib/core/util.js	`89.47% <0.00%> (-9.22%)`	⬇️
lib/api/readable.js	`82.75% <0.00%> (-8.63%)`	⬇️
lib/handler/redirect.js	`87.34% <0.00%> (-7.60%)`	⬇️
lib/proxy-agent.js	`87.30% <0.00%> (-6.35%)`	⬇️
lib/core/request.js	`95.33% <0.00%> (-4.67%)`	⬇️
lib/mock/mock-utils.js	`96.92% <0.00%> (-3.08%)`	⬇️
lib/fetch/dataURL.js
lib/fetch/request.js
... and 11 more

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 5ca25c2...1b4bc6b. Read the comment docs.

lib/balanced-pool.js

ronag

I think this could use more comments in the code explaining the algorithm and also maybe some references to the paper.

lib/balanced-pool.js

jodevsa · 2021-10-30T22:05:13Z

I still have to fix the comments and make them better

lib/balanced-pool.js

mcollina

Could you test this against a few scenarios:

an upstream is unavailable from the start, the algorithm should successfully route requests to working peers.
the upstream become online. Request should start be fulfilled by the new upstream.
the upstream go offline again. Requests should be routed to other upstreams.

lib/balanced-pool.js

ronag · 2021-10-31T13:23:38Z

Rebase main and you will get kNeedDrain to use instead of busy.

ronag · 2021-11-19T09:01:49Z

@jodevsa you still intend to work on this?

jodevsa · 2021-11-19T11:31:04Z

@ronag yeah, but no time right now :( I might be able to work on it on Saturday. Please feel free to take over if this thing is blocking you :)

ronag · 2021-11-19T11:35:38Z

@ronag yeah, but no time right now :( I might be able to work on it on Saturday. Please feel free to take over if this thing is blocking you :)

Not blocking. Just curious.

test/balanced-pool.js

jodevsa · 2021-12-05T09:01:07Z

@mcollina please don't review yet. I need to make the tests more understanable

lib/balanced-pool.js

kibertoad · 2022-06-05T11:36:23Z

Can I help with this one?

mcollina · 2022-06-05T11:43:57Z

Go for it!

kibertoad · 2022-06-05T14:53:40Z

@mcollina are the only remaining items the unaddressed comments and conflicts, or there is something else?

mcollina · 2022-07-05T18:16:23Z

Unsure, this PR is still a draft from the author.

jodevsa · 2022-07-09T12:17:05Z

Hi @mcollina, @ronag,

This PR is ready for reviewing again 🎉

mcollina

lgtm

jodevsa · 2022-07-14T00:20:57Z

Hi guys, what are the next steps for getting this merged? are we missing more reviewers?

mcollina · 2022-07-14T10:32:22Z

Having @ronag would have been nice but he looks busy. Let's ship this.

ronag · 2022-07-14T10:48:17Z

lib/balanced-pool.js

+        pool[kWeight] = Math.max(1, pool[kWeight] - this[kErrorPenalty])
+        this._updateBalancedPoolStats()
+      }
+    })


This event handlers need to be removed in removeUpstream.

good find! I'll open another PR to address your comments

ronag · 2022-07-14T10:48:55Z

lib/balanced-pool.js

@@ -100,10 +145,42 @@ class BalancedPool extends PoolBase {
      return
    }

-    this[kClients].splice(this[kClients].indexOf(dispatcher), 1)
-    this[kClients].push(dispatcher)
+    const allClientsBusy = this[kClients].map(pool => pool[kNeedDrain]).reduce((a, b) => a && b, true)


* fixes * more fixes * add test * remove console.log * rename startingWeightPerServer to maxWeightPerServer * add another test

jodevsa commented Oct 20, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

mcollina reviewed Oct 21, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

jodevsa requested a review from mcollina October 22, 2021 17:42

jodevsa changed the title ~~WIP: use weighted round robin in balancedPool~~ Use weighted round robin in balancedPool Oct 22, 2021

ronag reviewed Oct 23, 2021

View reviewed changes

mcollina reviewed Oct 23, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

lib/balanced-pool.js Outdated Show resolved Hide resolved

ronag force-pushed the main branch 2 times, most recently from a2ad318 to 904e045 Compare October 29, 2021 12:45

jodevsa requested review from mcollina and ronag October 30, 2021 21:59

jodevsa commented Oct 30, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

mcollina reviewed Oct 31, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

ronag force-pushed the main branch from 94ba37e to 3b1b03c Compare November 18, 2021 10:12

ronag force-pushed the main branch 3 times, most recently from 7e971bc to c4826ec Compare November 27, 2021 20:14

jodevsa requested a review from mcollina December 5, 2021 01:45

jodevsa commented Dec 5, 2021

View reviewed changes

test/balanced-pool.js Outdated Show resolved Hide resolved

jodevsa commented Dec 5, 2021

View reviewed changes

test/balanced-pool.js Outdated Show resolved Hide resolved

jodevsa commented Dec 5, 2021

View reviewed changes

test/balanced-pool.js Outdated Show resolved Hide resolved

ronag marked this pull request as draft December 5, 2021 09:16

ronag reviewed Dec 5, 2021

View reviewed changes

lib/balanced-pool.js Outdated Show resolved Hide resolved

ronag reviewed Dec 5, 2021

View reviewed changes

lib/balanced-pool.js Show resolved Hide resolved

ronag reviewed Dec 5, 2021

View reviewed changes

lib/balanced-pool.js Show resolved Hide resolved

ronag force-pushed the main branch 2 times, most recently from 63eab1d to 47eb207 Compare December 12, 2021 12:27

ronag force-pushed the main branch from ced66e9 to f569c49 Compare January 12, 2022 11:31

jodevsa closed this Jul 9, 2022

fixes

98743fa

jodevsa reopened this Jul 9, 2022

jodevsa added 2 commits July 9, 2022 15:05

more fixes

fda8da9

add test

7c0b2ac

jodevsa marked this pull request as ready for review July 9, 2022 12:16

remove console.log

a05c547

jodevsa requested a review from ronag July 9, 2022 12:19

jodevsa added 2 commits July 9, 2022 15:24

rename startingWeightPerServer to maxWeightPerServer

467f847

add another test

1b4bc6b

mcollina approved these changes Jul 11, 2022

View reviewed changes

mcollina changed the title ~~Use weighted round robin in balancedPool~~ feat: use weighted round robin in balancedPool Jul 11, 2022

mcollina merged commit 99205ec into nodejs:main Jul 14, 2022

ronag reviewed Jul 14, 2022

View reviewed changes

metcoder95 pushed a commit to metcoder95/undici that referenced this pull request Dec 26, 2022

feat: use weighted round robin in balancedPool (nodejs#1069)

44b8177

* fixes * more fixes * add test * remove console.log * rename startingWeightPerServer to maxWeightPerServer * add another test

crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024

feat: use weighted round robin in balancedPool (nodejs#1069)

41acbc1

* fixes * more fixes * add test * remove console.log * rename startingWeightPerServer to maxWeightPerServer * add another test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use weighted round robin in balancedPool #1069

feat: use weighted round robin in balancedPool #1069

jodevsa commented Oct 20, 2021 •

edited

codecov-commenter commented Oct 20, 2021 •

edited

ronag left a comment •

edited

jodevsa commented Oct 30, 2021

mcollina left a comment

ronag commented Oct 31, 2021

ronag commented Nov 19, 2021

jodevsa commented Nov 19, 2021 •

edited

ronag commented Nov 19, 2021

jodevsa commented Dec 5, 2021

kibertoad commented Jun 5, 2022

mcollina commented Jun 5, 2022

kibertoad commented Jun 5, 2022

mcollina commented Jul 5, 2022

jodevsa commented Jul 9, 2022

mcollina left a comment

jodevsa commented Jul 14, 2022

mcollina commented Jul 14, 2022

ronag Jul 14, 2022

jodevsa Jul 14, 2022

ronag Jul 14, 2022

feat: use weighted round robin in balancedPool #1069

feat: use weighted round robin in balancedPool #1069

Conversation

jodevsa commented Oct 20, 2021 • edited

codecov-commenter commented Oct 20, 2021 • edited

Codecov Report

ronag left a comment • edited

Choose a reason for hiding this comment

jodevsa commented Oct 30, 2021

mcollina left a comment

Choose a reason for hiding this comment

ronag commented Oct 31, 2021

ronag commented Nov 19, 2021

jodevsa commented Nov 19, 2021 • edited

ronag commented Nov 19, 2021

jodevsa commented Dec 5, 2021

kibertoad commented Jun 5, 2022

mcollina commented Jun 5, 2022

kibertoad commented Jun 5, 2022

mcollina commented Jul 5, 2022

jodevsa commented Jul 9, 2022

mcollina left a comment

Choose a reason for hiding this comment

jodevsa commented Jul 14, 2022

mcollina commented Jul 14, 2022

ronag Jul 14, 2022

Choose a reason for hiding this comment

jodevsa Jul 14, 2022

Choose a reason for hiding this comment

ronag Jul 14, 2022

Choose a reason for hiding this comment

jodevsa commented Oct 20, 2021 •

edited

codecov-commenter commented Oct 20, 2021 •

edited

ronag left a comment •

edited

jodevsa commented Nov 19, 2021 •

edited