-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sql,*: remove DistributionTypeSystemTenantOnly #116559
sql,*: remove DistributionTypeSystemTenantOnly #116559
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 11 of 11 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
pkg/sql/distsql_physical_planner.go
line 153 at r1 (raw file):
const ( // DistributionTypeNone does not distribute a plan across multiple instances. DistributionTypeNone = iota
Since we're now down to only two options, let's clean up this a bit. Should we just remove DistributionType
altogether in favor of using distribute bool
argument in its place? Or we could replace this type to use bool
and rename two remaining options to something like LocalDistribution
and FullDistribution
. WDYT?
Uses of DistributionTypeSystemTenantOnly represent places where UA/MT performance may not match single-tenant performance. This replaces all uses of DistributionTypeSystemTenantOnly with DistributionTypeAlways. Epic: none Fixes cockroachdb#116534
This commit does the following renaming to better represent the options: - `DistributionTypeNone` -> `LocalDistribution` - `DistributionTypeAlways` -> `FullDistribution`. Release note: None
20470d4
to
5633695
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 7 of 7 files at r2, 19 of 19 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @stevendanna)
pkg/sql/distsql_physical_planner.go
line 153 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
Since we're now down to only two options, let's clean up this a bit. Should we just remove
DistributionType
altogether in favor of usingdistribute bool
argument in its place? Or we could replace this type to usebool
and rename two remaining options to something likeLocalDistribution
andFullDistribution
. WDYT?
I added another commit to do the renaming (while keeping the two-value enum).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps someone else from @cockroachdb/sql-queries-prs should take a look too.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball, @dt, @rharding6373, and @stevendanna)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 11 files at r1, 7 of 7 files at r2, 19 of 19 files at r3, all commit messages.
Reviewable status: complete! 2 of 0 LGTMs obtained (waiting on @dt, @rharding6373, and @stevendanna)
Thanks! bors r+ |
Build succeeded: |
Thanks @yuzefovich ! |
sql,*: remove DistributionTypeSystemTenantOnly
Uses of DistributionTypeSystemTenantOnly represent places where UA/MT performance may not match single-tenant performance.
This replaces all uses of DistributionTypeSystemTenantOnly with DistributionTypeAlways.
Epic: none
Fixes #116534
sql: rename two DistributionType options
This commit does the following renaming to better represent the options:
DistributionTypeNone
->LocalDistribution
DistributionTypeAlways
->FullDistribution
.Release note: None