Repository navigation
fix: count CNAME queries in dry-run estimates - #224
jiangyuer018 wants to merge 1 commit into
Conversation
TMHSDigital
left a comment
There was a problem hiding this comment.
Thanks, this is the right fix. QueriesPerName sums the same queriesPerType table that ResolveTypes reserves rate slots from, and ParseTypes already normalizes and dedupes -type, so the estimate now matches the wire. I ran go test ./... and golangci-lint run ./... on 217ea1f locally: all green, 0 lint issues. I've approved the CI runs.
One thing before this is ready: the old "one query per record type" wording is still in three places.
main.go:242: the-max-querieshelp text.docs/_includes/flags.mdis generated from it (andmain_docs_test.gochecks the two match), so after editingmain.go, regenerate it with the Makefile target (seeMakefile:44) rather than editing it by hand.README.md:437: "each entry costs one query per record type (two with the defaultA,AAAA)" needs the CNAME exception too. Without it, the second acceptance criterion on #223 is only half met.
Optional nit: the "same types as the resolver receives" case in TestQueriesPerName passes CNAME twice, which ParseTypes never produces. Either drop it or rename it to say what it pins down (duplicates are not deduped).
Once those land, mark it ready and I'll merge.
|
Thanks for this, and sorry for the churn: a docs pass on Your wording is fine too. Keep whichever you prefer when you rebase on |
The -max-queries help text (and the flag table generated from it) and the README's wordlist-size note still said one query per record type. A CNAME lookup sends three, as -dry-run and -rate now count. Wording from #224. Refs #223 Co-authored-by: jiangyuer018 <[email protected]> Co-Authored-By: Claude Opus 5.5 <[email protected]>
|
Thank you for this, @jiangyuer018. Your approach was right, and it's now on
Sorry for the churn while this was open, and thanks again for the contribution. |
Description
Fixes #223.
-dry-runnow uses the same per-record-type wire-query costs as the resolver. CNAME estimates include the A, AAAA and CNAME queries, so retry and duration estimates use the correct base count.Related Issue
Fixes #223
Type of Change
Checklist
Ethical Considerations
Additional Information
Added unit coverage for the shared DNS query-cost helper and a simulated dry-run regression covering A, AAAA, CNAME, retries and rate-based duration.
git diff --checkpasses. Localgo test -race ./...andmake lintcould not run because Go/Make are unavailable; this is a draft so CI can verify the change before review.