Support multiple DNS servers #131
Conversation
8624b29 to
964c488
Compare
|
Any specific reason why you are resolving lookups concurrently against all nameservers instead of the commonly used fallback strategy? I'd think there is a better way to make use of multiple nameservers instead of blindly asking each of them for a record. |
|
@CharlotteDunois Working on something like that locally where they is a interval between the queries rather then using this shotgun method. Main reason why I put it up in this form is because I had a discussion with @clue about whether it would be possible to do this with out without breaking BC. |
964c488 to
abc47f8
Compare
|
@CharlotteDunois updated the code to do the fallback and also shuffle the server list each resolve call |
|
@WyriHaximus Do you think randomly shuffling the list is a good idea considering shuffling isn't exactly a cheap operation? I'd think just going down the list consequtively using an integer referring the key would be much cheaper and deterministic. |
|
@CharlotteDunois In doubt about that, would prefer to spread the load of the DNS queries across available servers over always hitting the first server in the list first. Could also go for an |
|
@WyriHaximus That's not what I exactly meant. // nsID = current nameserver to use (starts with -1 and resets to 0)
// maxNsID = count($nameservers)
if(++$nsID >= $maxNsID) {
$nsID = 0;
}
$ns = $nameservers[$nsID];
return $ns->resolve(...); |
abc47f8 to
51fa3cf
Compare
|
@CharlotteDunois Ah doh, just updated the PR based on that idea |
src/Resolver/Resolver.php
Outdated
| $nameserver, | ||
| $query | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I don't really like queueing fallbacks eagerly. In most (rather non-exceptional) situations they are unnecessary and a waste of CPU cycles.
Instead a recursively used function which queues the same query for the next nameserver should be the preferred way.
However it's fine with me if the other maintainers think it's fine, I'm not blindly stuck on this.
There was a problem hiding this comment.
Oh it's also not correctly implemented. It's very possible to try to get a non-existent nameserver. Imagine $this->nameserverStart = 4 and $this->nameServerCount = 5. The counter will be increased to 4 + 5 and there you have your out of bound error.
There was a problem hiding this comment.
I think you're having an interesting discussion. I agree that sending queries to all servers at once should probably not be the default - but I can see some value in this strategy depending on the use case. As such, I wonder if it makes sense to move this logic somewhere else? For example, by preparing the ExecutorInterface and implement different strategies there?
There was a problem hiding this comment.
@WyriHaximus Thanks for working on this, I too would love to have this feature!
That being said, I don't think we should rush things here. I see there's some interesting discussion about the best strategy to use multiple DNS servers. On top of this, see also #6 (comment) for common system defaults and possible ways to configure this in the future.
As such, I would suggest we do not target the next stable release and instead get this ready in a future v1.x.0 feature release.
In the meantime, I think it makes sense to consider some API implications and see if we can get this prepared so that we do not have to introduce any BC breaks in future versions. What do you think? 👍
examples/04-multiple-servers.php
Outdated
| }, 'printf'); | ||
| } | ||
|
|
||
| $loop->addTimer(1.0, function() use ($names, $resolver) { |
There was a problem hiding this comment.
What's the motivation for these timers?
examples/04-multiple-servers.php
Outdated
|
|
||
| $names = array_slice($argv, 1); | ||
| if (!$names) { | ||
| $names = array('google.com', 'www.google.com', 'gmail.com'); |
There was a problem hiding this comment.
This example is a bit unclear, are we testing multiple servers or multiple queries or both at the same time and if so, what is the expected output, how do I know "it works" as a consumer?
src/Resolver/Resolver.php
Outdated
| private $executor; | ||
|
|
||
| public function __construct($nameserver, ExecutorInterface $executor) | ||
| public function __construct(array $nameservers, ExecutorInterface $executor) |
src/Resolver/Resolver.php
Outdated
| $nameserver, | ||
| $query | ||
| ); | ||
| }); |
There was a problem hiding this comment.
I think you're having an interesting discussion. I agree that sending queries to all servers at once should probably not be the default - but I can see some value in this strategy depending on the use case. As such, I wonder if it makes sense to move this logic somewhere else? For example, by preparing the ExecutorInterface and implement different strategies there?
Honestly? That's my primary goal here. Anything else is a huge bonus 😎 |
See #135 to prepare the internal |
👍! Yup will look at this soon enough after this :) |
51fa3cf to
8d7abf3
Compare
8d7abf3 to
aed932b
Compare
|
Ok the code changes are in, will add the readme documentation and class documentation next |
65e740b to
189526a
Compare
There was a problem hiding this comment.
Thanks for the update 👍
Tbqh I find it quite hard to give a reasonable review for this PR because I'm not sure what the use case for this strategy implementation is, so perhaps it really boils down to a lack of documentation? ¯\(ツ)/¯ It's my understanding that this strategy might be useful in some rare cases, but I wonder if it doesn't make more sense to start with common fallback strategies as discussed in #6 first?
What do you think about this?
|
What would be the use cases for a non-deterministic random executor executor? I feel like this constant shuffling and array popping has a too high cost (you can implement it cheaper!) for no use case I can see. I'd prefer the strategy we've discussed above or one from the linked issue. |
|
@CharlotteDunois yes I should have, doing that now. Same way I've done it in the previous iteration 👍 |
|
@clue my main concern with a fallback executor is that it would always reach out to the first server in the list first. Delayed cycling the requests over all the servers in the list spreads that load and increases response times. (And during development testing often (using a list of 5) it would never reach the end of the server list when getting a response.) I'll update the OP and add more reasoning into the commit message |
103cd22 to
5ffe027
Compare
Currently this component only supports a single DNS server so with reactphp#6 in mind I started working on adding multiple server support. But ran into the issue that if the first server on the list isn't responding it takes a while before falling over the the next server in the list. So with my work on reactphp/socket#196 in mind and specifically the basic principle of https://tools.ietf.org/html/rfc8305#section-3.1 the work shifted to sending the queries out with a delay between them rather then waiting for the first server to fail/succeed. On the plus side records will resolve faster when a server in the list is failing. On the down side it will send more queries thus generating a bit more network traffic.
5ffe027 to
a5ad7a1
Compare
|
@clue added reasoning to the PR and commit |
They are all designed to test every possible code path. But I'll look into breaking them up in more descriptive tests. |
This little bit of syntactical sugar makes it cleaner to use the Config class to load the system resolver configuration. Which right now is limited to only one nameserver. By introducing this factory method completing the implementation in reactphp#131 will also allow for instant multiple server support out of the box without any need for users to change code on their end to implement it.
This little bit of syntactical sugar makes it cleaner to use the Config class to load the system resolver configuration. Which right now is limited to only one nameserver. By introducing this factory method completing the implementation in reactphp#131 will also allow for instant multiple server support out of the box without any need for users to change code on their end to implement it.
Currently this component only supports a single DNS server so with #6 in mind I started working on adding multiple server support. But ran into the issue that if the first server on the list isn't responding it takes a while before falling over the the next server in the list. So with my work on reactphp/socket#196 in mind and specifically the basic principle of https://tools.ietf.org/html/rfc8305#section-3.1 the work shifted to sending the queries out with a delay between them rather then waiting for the first server to fail/succeed. On the plus side records will resolve faster when a server in the list is failing. On the down side it will send more queries thus generating a bit more network traffic.