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

refactor(balancer) drop the orderlist property #2748

Merged
merged 1 commit into from Sep 11, 2017
Merged

Conversation

Tieske
Copy link
Member

@Tieske Tieske commented Jul 30, 2017

This removes the orderlist property from the balancer entity. Due to a different implementation in the dns library, it is no longer required.

Reason:
Because multiple Kong nodes need the exact same ring-balancer, the orderlist was used to make sure they all used the same random data to set up the balancer.
This has now been replaced by using a separate randomizer in the dns library, which always will be seeded again, and with the same seed. This causes the exact same random set of numbers to be generated. This works because there is no need for uniqueness, there is only need for distribution of the data.

@Tieske Tieske self-assigned this Jul 30, 2017
@Tieske Tieske force-pushed the refactor/drop-order-list branch 3 times, most recently from b074f58 to 8bd3b3b Compare July 31, 2017 09:22
Copy link
Member

@thibaultcha thibaultcha left a comment

Choose a reason for hiding this comment

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

Very nice. Just left a couple of minor comments. As discussed, this should be scheduled for post 0.11.0/0.10.4 releases.

kong/kong.lua Outdated
@@ -308,6 +308,7 @@ function Kong.balancer()
current_try.port = addr.port

-- set the targets as resolved
ngx.log(ngx.DEBUG, "setting address (try ", addr.try_count, "): ",addr.ip,":",addr.port)
Copy link
Member

Choose a reason for hiding this comment

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

perf: on this hot code path, let's cache ngx.log and ngx.DEBUG which imply global accesses.

style: it seems like we are going over the 80 chars limit and missing a few spaces for readability. Let's use the following form instead:

  ngx.log(ngx.DEBUG, "setting address (try ", addr.try_count, "): ",
                     addr.ip, ":", addr.port)

(with globals caching ofc)

@@ -167,7 +186,7 @@ dao_helpers.for_each_dao(function(kong_config)
assert.are.equal(requests/2, count1)
assert.are.equal(requests/2, count2)
end)
it("adding a target", function()
it("adding a target #only", function()
Copy link
Member

Choose a reason for hiding this comment

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

#only ^

@thibaultcha thibaultcha added pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release) and removed pr/status/please review labels Jul 31, 2017
@Tieske Tieske force-pushed the refactor/drop-order-list branch 2 times, most recently from 92b67fa to 90466c6 Compare August 31, 2017 13:19
@@ -27,7 +27,7 @@ dependencies = {
"luacrypto == 0.3.2",
"luasyslog == 1.0.0",
"lua_pack == 1.0.5",
"lua-resty-dns-client == 0.6.0",
"lua-resty-dns-client == 0.6.1",
Copy link
Member

Choose a reason for hiding this comment

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

Should we bump this to 0.6.2 now? Considering master is already there.

Copy link
Member Author

Choose a reason for hiding this comment

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

@thibaultcha Done!

Tieske added a commit to Kong/docs.konghq.com that referenced this pull request Sep 11, 2017
@Tieske Tieske merged commit 6eb534b into next Sep 11, 2017
@Tieske Tieske deleted the refactor/drop-order-list branch September 11, 2017 11:52
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Dec 18, 2017
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 12, 2018
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018
thibaultcha pushed a commit to Kong/docs.konghq.com that referenced this pull request Jan 16, 2018
thibaultcha pushed a commit that referenced this pull request Jan 16, 2018
This removes the `orderlist` property from the balancer entity. Due to a different implementation in the dns library, it is no longer required.

from #2748
thibaultcha pushed a commit that referenced this pull request Jan 19, 2018
This removes the `orderlist` property from the balancer entity. Due to a different implementation in the dns library, it is no longer required.

from #2748
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr/ready (but hold merge) No more concerns, but do not merge yet (probably a conflict of interest with another PR or release)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants