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

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

Merged
merged 7 commits into from
Jan 7, 2021

Conversation

kai08-cn
Copy link
Contributor

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

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
@@ -55,6 +55,7 @@ end

function _M.new(up_nodes, upstream)
local str_null = str_char(0)
local last_server_index
Copy link
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.

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
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
Member

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

@moonming
Copy link
Member

moonming commented Jan 5, 2021

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