(#127) Degrading retry DNS resolve. Needs tests #133
Conversation
There was a problem hiding this comment.
This looks great. I like the approach and left some nitpicks in the code.
Other than that, let's as you say add some tests for this.
I'd be happy to go over the snapshotting mechanism with you, but first could you tell me what part is unclear? (unless of course it's the whole thing :) )
| Pre-pull request checklist: | ||
| - Run tests: `cargo test` | ||
| - Check the formatting: `cargo fmt -- --check` | ||
| - Run linter: `cargo clippy --all-targets --all-features -- -D warnings` |
There was a problem hiding this comment.
Thank you very much for your documentation efforts! I think other contributors will find these very helpful.
| let ips = ips | ||
| .into_iter() | ||
| .filter(|ip| { | ||
| let mut pending = pending.lock().unwrap().clone(); |
There was a problem hiding this comment.
Do you think we can move the mutex lock outside of the loop? Otherwise it'll constantly be locking/unlocking for every ip in the list. Or is there a consideration here I'm not seeing?
There was a problem hiding this comment.
Good idea. Will do.
|
There's no real confusion (yet) regarding snapshotting. I just need to spend some time looking at it which I haven't really done so far. Since you're okay with the approach above, I'll dig into the testing. Thanks for the review and comments. |
|
Hey, this was closed by mistake and I don't seem to be able to re-open it. Please feel free to re-open or make another PR with this if you wish. My apologies! |
Hi Friends,
I've only tested this manually. I still need to grok how the snapshot testing infrastructure works and then figure out how to shoehorn some dns resolve failures into a test. While I do that, I figure I might as well send this for at least a code review because I moved some stuff around and my approach should be reviewed as well.
I moved adding ips to
pendingout of theClient::resolvefunction and into the block defininghandle. All ips are added to pending along with a counter for how many times the client has been asked to resolve the ip. If it is the first attempt or an attempt that is a power of 2, an actual dns call will be made. If an ip is resolved it will be removed frompending. This will indirectly give the increasing retry timeout suggested in the previous comment. e.g. 2, 4, 8, 16 seconds.Let me know if any of this looks weird. I am still getting used to borrowing/clone etc. and I have discovered doing simple stuff with numbers in rust makes me want to stab hot pokers into my eyeballs. I think I just need to get used to it.