WitMaxConcurrency rewrite#1462
Conversation
tanmayrauth
left a comment
tanmayrauth
left a comment
There was a problem hiding this comment.
LGTM! thanks foe the fix.
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
The shape of this is right — canonicalizing the typo'd name behind a deprecated WitMaxConcurrency alias is how we've handled this elsewhere, the delegation is a clean pass-through, and dropping the stale TODO plus updating the call site in ExecuteCompactionGroup are exactly the right cleanup.
I'd hold on the name itself before we merge, though. WithScanMaxConcurrency gets baked in the moment the alias ships, and it cuts against two conventions at once: every other ScanOption drops the Scan infix, and every other concurrency option is With<Operation>Concurrency with no Max. You flagged wanting to discuss this, and I think it's worth it — the clean name WithMaxConcurrency is taken by the orphan-cleanup option, so we either rename that one to free it up here, or go with WithScanConcurrency to match the family. If Max is intentional, let's just note why in the doc.
Two smaller asks once the name's settled: a short table-driven test for the new name + alias parity (scan_options_test.go covers only WithSelectedFields right now), and "backwards" → "backward" in the alias doc to match the rest of the package.
Settle the name and I'm happy to take another pass.
| // WithScanMaxConcurrency sets the maximum concurrency for table scan and plan | ||
| // operations. When unset it defaults to runtime.GOMAXPROCS. | ||
| func WitMaxConcurrency(n int) ScanOption { | ||
| func WithScanMaxConcurrency(n int) ScanOption { |
There was a problem hiding this comment.
This is the one thing I'd want to settle before we merge, since the name gets locked in by the deprecation alias — re-changing it later means another deprecation cycle.
WithScanMaxConcurrency is a bit of an odd duck in two ways. Every other ScanOption drops the Scan infix (WithLimit, WithRowFilter, WithSnapshotID, WithCaseSensitive), and every other concurrency option follows With<Operation>Concurrency with no Max (WithAddFilesConcurrency, WithOverwriteConcurrency, WithCompactionScanConcurrency). So this name cuts against both conventions at once.
The clean name is WithMaxConcurrency, but orphan_cleanup.go:118 owns it. Two ways out: rename that one to WithCleanupMaxConcurrency and free up WithMaxConcurrency here (a second breaking rename, but the namespace ends up right), or go with WithScanConcurrency to match the With<Operation>Concurrency family. If Max is deliberate — signalling an upper bound vs a fixed count — I'd say so in the doc. wdyt?
| // backwards compatibility with the pre-existing typo'd name. | ||
| // | ||
| // Deprecated: use [WithScanMaxConcurrency]. | ||
| func WitMaxConcurrency(n int) ScanOption { |
There was a problem hiding this comment.
Neither name has a test — scan_options_test.go only covers WithSelectedFields today. A ~10-line table-driven case covering n>0 (sets scan.concurrency), n<=0 (returns noopOption), and alias/canonical parity would lock the forwarding down so the alias can't silently drift. wdyt?
| } | ||
|
|
||
| // WitMaxConcurrency is a deprecated alias for [WithScanMaxConcurrency], kept for | ||
| // backwards compatibility with the pre-existing typo'd name. |
There was a problem hiding this comment.
tiny thing while we're here — the codebase uses "backward compatibility" without the s (orphan_cleanup.go:163, name_mapping.go:134). I'd match that.
3 participants
(I think we should have a discussion on this one)
We had a ScanOption named
WitMaxConcurrency, which is obviously a typo. But, the correct name conflicts with an option in orpan_cleanups.go calledWithMaxConcurrency.I've renamed
WithMaxConcurrency->WithScanMaxConcurrency. Ideally, this conflict shouldn't happen in the first place, but we'll have to do some package renames to make that happen.