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

bugfix: fix a typo in chash.reinit #25

Merged
merged 2 commits into from
Feb 25, 2019
Merged

bugfix: fix a typo in chash.reinit #25

merged 2 commits into from
Feb 25, 2019

Conversation

ArchangelSDY
Copy link
Contributor

newnodes should be assigned to self.nodes rather than self.newnodes.

@ArchangelSDY
bugfix: fix a typo in chash.reinit
5fb57d8 
`newnodes` should be assigned to `self.nodes` rather than `self.newnodes`.
@agentzh
Copy link
Member

agentzh commented Feb 19, 2019

@doujiang24 Will you have a look at this? Thanks.

doujiang24
doujiang24 approved these changes Feb 21, 2019
View reviewed changes
Copy link
Member

@doujiang24 doujiang24 left a comment

Choose a reason for hiding this comment

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

@ArchangelSDY nice catch, will you please add a test case for it?

@ArchangelSDY
Copy link
Contributor Author

@doujiang24 Sure, updated.

@doujiang24
Copy link
Member

@ArchangelSDY Thanks a lot :)

@ArchangelSDY
Copy link
Contributor Author

@doujiang24 Could you merge this PR and tag a new release so that we can pick this change in downstream projects?

@doujiang24 doujiang24 merged commit 0fb8a79 into openresty:master Feb 25, 2019
@doujiang24
Copy link
Member

@ArchangelSDY
merged & added a new release tag: v0.02rc5, thanks for your PR :)

@ArchangelSDY ArchangelSDY mentioned this pull request Feb 25, 2019
Merged
ArchangelSDY added a commit to ArchangelSDY/ingress-nginx that referenced this pull request Feb 25, 2019
@ArchangelSDY
Upgrade openresty/lua-resty-balancer
db4d012 
This fixes bug in chash:reinit which prevents endpoints from being
updated correctly.

See openresty/lua-resty-balancer#25
@ElvinEfendi
Copy link
Contributor

What's the impact of this bug? As long as a user you aren't relying on nodes to be in sync (covered at https://github.com/openresty/lua-resty-balancer/pull/25/files#diff-cad7d8fbdf9d2fb89f5ee95f0b73dff7R338), there should not be any issue, right?

@ArchangelSDY
Copy link
Contributor Author

That's right as long as you don't need to read self.nodes. I've added a comment in kubernetes/ingress-nginx#3809 (comment) .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doujiang24 doujiang24 doujiang24 approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@ArchangelSDY @agentzh @doujiang24 @ElvinEfendi