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

dnsdist: Add support for setting the server selection policy on a per pool basis #5113

Merged
merged 3 commits into from Mar 7, 2017

Conversation

@RobinGeuze
Copy link
Contributor

@RobinGeuze RobinGeuze commented Mar 6, 2017

Short description

Add support to dnsdist for setting the server selection policy on a per pool basis, allowing for using different strategies for different pools.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled and tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
@RobinGeuze RobinGeuze changed the title Add support for setting the server selection policy on a per pool basis dnsdist: Add support for setting the server selection policy on a per pool basis Mar 6, 2017
Copy link
Member

@rgacogne rgacogne left a comment

Nice PR, thank you! A few comments, and please try to use nullptr instead of NULL as it prevents mistakes.

setLuaSideEffect();
auto localPools = g_pools.getCopy();
setPoolPolicy(localPools, pool, std::make_shared<ServerPolicy>(policy));
});

This comment has been minimized.

@rgacogne

rgacogne Mar 6, 2017
Member

The issue here is that you are updating a local copy of the pools but not updating the global state. It won't work if the pool doesn't already exists and setPoolPolicy() creates it, because it will only exist in the local copy. You need to do g_pools.setState(localPools) afterwards. Same issue in setPoolServerPolicyLua().

@@ -692,11 +692,23 @@ std::shared_ptr<ServerPool> createPoolIfNotExists(pools_t& pools, const string&
if (!poolName.empty())
vinfolog("Creating pool %s", poolName);
pool = std::make_shared<ServerPool>();
pool->policy = NULL;

This comment has been minimized.

@rgacogne

rgacogne Mar 6, 2017
Member

Just make sure it's default initialized to nullptr by replacing:
std::shared_ptr<ServerPolicy> policy;
by
std::shared_ptr<ServerPolicy> policy{nullptr};

Use nullptr instead of NULL
Statically initialize policy to nullptr in Pool object
Actually set the global state for the pools after setting a new
server selection policy on a pool.
Copy link
Member

@rgacogne rgacogne left a comment

Looks good, waiting for travis 👍

@rgacogne rgacogne merged commit 1896dee into PowerDNS:master Mar 7, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.