Add the name resolver API #2285
Conversation
arjan-bal
force-pushed
the
nameresolver-api
branch
from
2867b3f to
3c30ab8
Compare
arjan-bal
force-pushed
the
nameresolver-api
branch
from
3c30ab8 to
07eb017
Compare
| pub fn new(mut config: BackoffConfig) -> Self { | ||
| // Adjust params to get them in valid ranges. | ||
| // 0 <= base_dealy <= max_delay | ||
| config.base_delay = config.base_delay.min(config.max_delay); |
There was a problem hiding this comment.
This doesn't ensure it isn't negative (max(0)) and doesn't validate max_delay at all.
Also, maybe new should return a Result instead and fail if things are out of range, rather than silently "fixing" the errors.
| let mut next_delay = self.next_delay_secs.lock().unwrap(); | ||
| *next_delay = self.config.base_delay.as_secs_f64(); |
There was a problem hiding this comment.
| let mut next_delay = self.next_delay_secs.lock().unwrap(); | |
| *next_delay = self.config.base_delay.as_secs_f64(); | |
| self.next_delay_secs.set(self.config.base_delay.as_secs_f64()); |
|
|
||
| /// The delay for the next retry, without the random jitter. Store as f64 | ||
| /// to avoid rounding errors. | ||
| next_delay_secs: Mutex<f64>, |
There was a problem hiding this comment.
Why does this need a mutex? It seems like we should generally only be accessing these serially.
| /// to prevent excessive re-resolution. | ||
| static MIN_RESOLUTION_INTERVAL_MS: AtomicU64 = AtomicU64::new(30_000); // 30 seconds | ||
|
|
||
| pub fn get_resolving_timeout() -> Duration { |
There was a problem hiding this comment.
These functions should presumably have the comments above, since they are public but the variables are not. But the module is not public, so it's not too important for now.
| let (resolve_now_tx, mut resolve_now_rx) = tokio::sync::mpsc::unbounded_channel::<()>(); | ||
| let (update_error_tx, update_error_rx) = | ||
| tokio::sync::mpsc::unbounded_channel::<Result<(), String>>(); |
There was a problem hiding this comment.
Should this use a Notify instead?
| /// Asks the resolver to obtain an updated resolver result, if | ||
| /// applicable. | ||
| /// | ||
| /// This is useful for pull-based implementations to decide when to |
There was a problem hiding this comment.
I believe we have standardized on poll and watch for the two resolver types.
| /// Called serially by the channel to do work after the work scheduler's | ||
| /// schedule_work method is called. |
There was a problem hiding this comment.
"to do work" -> "to allow access to ChannelController" or something?
| /// returns an empty endpoint list but a valid service config may set | ||
| /// to this to something like "no DNS entries found for <name>". | ||
| pub resolution_note: Option<String>, |
There was a problem hiding this comment.
Is there some requirement for using this? Like should it only be set if endpoints is empty? I.e. should we delete this and use an Err() from endpoints instead?
| pub resolution_note: Option<String>, | ||
| } | ||
|
|
||
| impl Default for ResolverUpdate { |
There was a problem hiding this comment.
Can this be derived? Or no because of Result? If the latter, is there some way to partially derive?
| /// Creates and returns an instance of a DNSResolver, optionally | ||
| /// configured by the ResolverOptions struct. This method may return an | ||
| /// error if it fails to create the DNSResolver. | ||
| fn get_dns_resolver(&self, opts: ResolverOptions) -> Result<Box<dyn DnsResolver>, String>; |
There was a problem hiding this comment.
Does it complicate things that this is fallible? I think we could make it infallible if we wanted?
No description provided.