Skip to content

fix: make set_more_retries() work when upstream_type is chash#2676

Merged
tokers merged 7 commits into
apache:masterfrom
kai08-cn:master
Jan 7, 2021
Merged

fix: make set_more_retries() work when upstream_type is chash#2676
tokers merged 7 commits into
apache:masterfrom
kai08-cn:master

Conversation

@kai08-cn

@kai08-cn kai08-cn commented Nov 9, 2020

Copy link
Copy Markdown
Contributor

chash will pick next if last doesn't work to enable set_more_retries().

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?

@kai08-cn kai08-cn changed the title chash pick next if last doesn't work make set_more_retries() work when upstream_type is chash Nov 9, 2020
@kai08-cn kai08-cn changed the title make set_more_retries() work when upstream_type is chash fix:make set_more_retries() work when upstream_type is chash Nov 9, 2020
@kai08-cn kai08-cn changed the title fix:make set_more_retries() work when upstream_type is chash fix: make set_more_retries() work when upstream_type is chash Nov 9, 2020
Comment thread apisix/balancer/chash.lua Outdated

function _M.new(up_nodes, upstream)
local str_null = str_char(0)
local last_server_index

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The last_server_index should be in ctx scope, not the upstream one. We should not share the same index across different requests to an upstream.

Comment thread apisix/balancer/chash.lua Outdated
local id = picker:find(chash_key)
local id
if ctx.balancer_try_count > 1 and ctx.last_server_index then
id, ctx.last_server_index = picker:next(ctx.last_server_index)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Better to use name like ctx.chash_last_server_index

@spacewander

Copy link
Copy Markdown
Member

Please write new test to verify the fix and make sure the tests are passed.

@moonming

moonming commented Jan 5, 2021

Copy link
Copy Markdown
Member

ping @zfs123

@tokers
tokers merged commit 500e42d into apache:master Jan 7, 2021
sysulq pushed a commit to sysulq/apisix that referenced this pull request Jan 15, 2021
@spacewander spacewander mentioned this pull request Feb 4, 2021
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.

4 participants