Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new “static/manual” SRV resolver implementation that returns pre-configured SRV records without performing DNS lookups, plus a small client convenience constructor and updated integration test coverage.
Changes:
- Introduce
resolver::manual::{StaticResolver, StaticSrvRecord}to bypass DNS SRV lookups. - Add
SrvClient::new_with_static_resolverconvenience constructor forStaticResolver. - Extend the single-record SRV lookup integration test to exercise the new resolver.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/resolver/mod.rs |
Exposes the new manual resolver module. |
src/resolver/manual.rs |
Implements the static resolver + record type and unit tests. |
src/client/mod.rs |
Adds new_with_static_resolver convenience constructor and imports StaticResolver. |
tests/test_simple_lookup_srv_single.rs |
Adds coverage path using StaticResolver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn get_srv_records_unordered( | ||
| &self, | ||
| _srv: &str, | ||
| ) -> Result<(Vec<Self::Record>, Instant), Self::Error> { | ||
| Ok((self.get_static_srv_records_unordered(), Instant::now())) | ||
| } |
There was a problem hiding this comment.
get_srv_records_unordered returns Instant::now() as valid_until. Because cache validity checks use Instant::now() <= valid_until (see src/client/cache.rs:22), this will almost always make caches immediately invalid and force a refresh on every operation. Consider returning a far-future valid_until (using a checked add with a large Duration) so static records can actually be cached.
There was a problem hiding this comment.
This would be trivial to add if an Instant::MAX existed, or, perhaps saturating addition on Instant, but those don't exist (at least, not yet: rust-lang/rust#71224). In the meantime, I think the current behavior is somewhat suboptimal but still acceptable given the potential use cases.
1975a5d to
9c8c60f
Compare
ping-deshaw2
left a comment
There was a problem hiding this comment.
LGTM, always a fan of more test coverage!
This adds a "static" resolver that bypasses DNS SRV resolution altogether and directly returns whatever you configure it with.
This is useful for, say, setting up a quick development instance of an SRV-based client-server system without needing to set up dummy SRV records first. Also nice to have when debugging (e.g., "is it DNS?").
The module for this is called
manualto signal that you'll be "manually" supplying the targets. Unfortunately,staticis a reserved keyword and I'd rather not putpub mod r#staticin the public API.I've also added some methods specific to this resolver to make working with it easier (e.g.,
get_static_srv_records,get_static_srv_records_unordered). The doc comments for both should (hopefully) explain why they're slightly nicer than their more generic, async counterparts meant for the resolvers that actually perform DNS resolution.I'd like to sneak this in before the 1.0 release at #36.