szmarczak / cacheable-lookup

A cacheable dns.lookup(…) that respects TTL :tada:
MIT License
193 stars 29 forks source link

New option to return only one ip when we resolve to multiple addresses #18

Closed cesarfd closed 4 years ago

cesarfd commented 4 years ago

This is kind of a corner case. At our company we use Got to make several thousand API calls/s to our partners. In our case, some of those partners domains resolve to multiple ip addresses (one of them to more than 70).

Obviously we need a DNS caching mechanism, but the problem is, when a domain resolves to multiple ip's, the caching just returns a random address each time. This is fine when the traffic is low but in our case this decreases performance as the system is constantly opening up new connections to new ips and the old ones don't get reused even though we set them to keepalive because they are targeting different addresses.

This option is just a single flag that, when enabled always returns the first entry. Obviously it's disabled by default.

szmarczak commented 4 years ago

You can override

https://github.com/szmarczak/cacheable-lookup/blob/6ed7e37ccffe7cda06140d20d6b0262e97bf36eb/source/index.js#L267-L269

class YourOwnCacheableLookup extends CacheableLookup {
    _getEntry(entries) {
        return entries[0];
    }
}

the old ones don't get reused even though we set them to keepalive

I don't think that's possible. Agents don't remember IPs but hostnames. So if there's a free connection, it should use one no matter the IP address. Are you using HTTP2? If so, please upgrade to http2-wrapper@1.0.0-beta.4.5.

cesarfd commented 4 years ago

I don't think that's possible.

I'm confused, you're saying the patch wouldn't work or that the concept is wrong? We monkey patched node's dns lookup function ourselves back when we were still using the old request library and it seemed to work fine.

Agents don't remember IPs but hostnames. So if there's a free connection, it should use one no matter the IP address. Are you using HTTP2? If so, please upgrade to http2-wrapper@1.0.0-beta.4.5.

We use both http 1.1 and h2, depending on the partner. We'd just set thehttp2 flag on and let Got negotiate. Should I wait to the next release of http2-wrapper then?

szmarczak commented 4 years ago

Should I wait to the next release of http2-wrapper then?

You don't need to. Just make sure you're running http2-wrapper@1.0.0-beta.4.5. What's the result of npm ls http2-wrapper?

I'm confused

This:

opening up new connections to new ips and the old ones don't get reused

Can you provide an example where connections are not reused?

szmarczak commented 4 years ago

@cesarfd Fixed in b2348d5aede36bdf625eba2aa17ca4dedb74c62b

szmarczak commented 4 years ago

Thanks for trying to fix this. I really apprecaite it! :raised_hands: Further investigation led to the discovery of some bugs, and I had to default return entries[0] to comply with the Node.js API.