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

add ewma balancer #2001

Merged
merged 20 commits into from
Aug 29, 2020
Merged

add ewma balancer #2001

merged 20 commits into from
Aug 29, 2020

Conversation

redynasc
Copy link
Contributor

@redynasc redynasc commented Aug 5, 2020

What this PR does / why we need it:

ewma is a different balancing implementation that will generate a weight for every backend IP based on the last server response time, basically it tries to dispatch more requests to the backends that reply faster, supposing that they are less loaded.

fix #1996

Pre-submission checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible?

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

real request for testing

apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
bin/apisix Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
@membphis
Copy link
Member

membphis commented Aug 6, 2020

@redynasc please take a look at the output of CI: https://github.com/apache/apisix/pull/2001/checks?check_run_id=952937828

+ luacheck -q apisix
Checking apisix/balancer/ewma.lua                 7 warnings

    apisix/balancer/ewma.lua:146:16: unused function calculate_slow_start_ewma
    apisix/balancer/ewma.lua:176:11: unused variable k
    apisix/balancer/ewma.lua:176:13: unused variable _
make: *** [lint] Error 1
    apisix/balancer/ewma.lua:192:21: variable ewma_score is never accessed
    apisix/balancer/ewma.lua:196:101: line is too long (102 > 100)
    apisix/balancer/ewma.lua:206:5: right side of assignment has less values than left side expects
    apisix/balancer/ewma.lua:225:101: line is too long (102 > 100)
+ exit 1

apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
@membphis
Copy link
Member

membphis commented Aug 6, 2020

@redynasc if we have fixed them, we can click the resolve ... button by ourselves.

image

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

it seems much better now.

apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
t/node/ewma.t Outdated Show resolved Hide resolved
t/node/ewma.t Outdated Show resolved Hide resolved
t/node/ewma.t Outdated Show resolved Hide resolved
t/node/ewma.t Show resolved Hide resolved
apisix/balancer/ewma.lua Show resolved Hide resolved
apisix/balancer/ewma.lua Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Show resolved Hide resolved
@redynasc redynasc requested a review from membphis August 12, 2020 00:56
@moonming
Copy link
Member

@membphis please take a look, and I think we need more test cases, WDYT?

@moonming
Copy link
Member

@redynasc For the code of other projects, we cannot add ASF header, please take a look at https://github.com/apache/skywalking/blob/master/LICENSE#L204, which is the correct way to deal with license issue.

doc/admin-api.md Outdated Show resolved Hide resolved
apisix/balancer/ewma.lua Show resolved Hide resolved
apisix/balancer/ewma.lua Outdated Show resolved Hide resolved
@membphis
Copy link
Member

@redynasc we need more test cases

@moonming
Copy link
Member

@redynasc ping

@moonming
Copy link
Member

@redynasc any update?

@redynasc
Copy link
Contributor Author

add one new test case,It is difficult to add more test items, any suggestions?

@membphis
Copy link
Member

membphis commented Aug 23, 2020

@redynasc it seems that the new test case is unstable. we need to fix this before we merge this PR

#   Failed test 'TEST 3: about frequency - response_body - response is expected (repeated req 0, req 0)'
#   at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"count":1,"port":"1980"}
# '
#     expected: '{"count":1,"port":"1981"}
# '
# Looks like you failed 1 test of 9.
t/node/ewma.t ............................ 

@redynasc
Copy link
Contributor Author

@redynasc it seems that the new test case is unstable. we need to fix this before we merge this PR

#   Failed test 'TEST 3: about frequency - response_body - response is expected (repeated req 0, req 0)'
#   at /home/runner/work/apisix/apisix/test-nginx/lib/Test/Nginx/Socket.pm line 1589.
#          got: '{"count":1,"port":"1980"}
# '
#     expected: '{"count":1,"port":"1981"}
# '
# Looks like you failed 1 test of 9.
t/node/ewma.t ............................ 

Do I need to care about the following errors in test?
2020/08/24 03:45:37 [error] 32670#32670: init_by_lua error: /usr/share/lua/5.1/tinyyaml.lua:698: bad argument #1 to 'sfind' (string expected, got nil)

@moonming
Copy link
Member

the license issue is not fixed, which is MUST fixed before next release.

@redynasc
Copy link
Contributor Author

the license issue is not fixed, which is MUST fixed before next release.

I added the "Apache ApiSix Subcomponent" section to the license file
I don't know how to solve this problem and need help

@moonming
Copy link
Member

moonming commented Aug 27, 2020 via email

@redynasc
Copy link
Contributor Author

redynasc commented Aug 28, 2020 via email

@membphis membphis dismissed moonming’s stale review August 29, 2020 15:13

fix license later

@membphis membphis merged commit 98bb593 into apache:master Aug 29, 2020
@membphis
Copy link
Member

@redynasc merged, many thx

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.

feat: new load balancing algorithm ewma for upstream node
3 participants