sql: add closest-instance physical planning#98468
sql: add closest-instance physical planning#98468craig[bot] merged 2 commits intocockroachdb:masterfrom
Conversation
|
This change is |
knz
left a comment
There was a problem hiding this comment.
i like it 💯 - but let's see what our sql queries folk think of it.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @rharding6373 and @yuzefovich)
rharding6373
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
yuzefovich
left a comment
There was a problem hiding this comment.
I agree, nice work!
Reviewed 2 of 2 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status:
pkg/roachpb/metadata.go line 615 at r1 (raw file):
} // SharedPrefix returns the number of this locality's tiers which match those
nit: something sounds off in "those the passed locality", like a preposition "of" is missing or something.
pkg/sql/distsql_physical_planner.go line 1381 at r2 (raw file):
} // If we were able to determine the region information for at least some
nit: this comment needs a minor adjustment now, probably just s/region/locality/g.
Release note: none. Epic: none.
This changes physical planning, specifically how the SQL instance for a given KV node ID is resolved, to be more generalized w.r.t. different locality tier taxonomies. Previously this function had a special case that checked for, and only for, a specific locality tier with the key "region" and if it was found, picked a random instance from the subset of instances where their value for that matched the value for the KV node. Matching on and only on the "region" tier is both too specific and not specific enough: it is "too specific" in that it requires a tier with the key "region" to be used and to match, and is "not specific enough" in that it simultaneously ignores more specific locality tiers that would indicate closer matches (e.g. subregion, AZ, data-center or rack). Instead, this change generalizes this function to identify the subset of instances that have the "closest match" in localities to the KV node and pick one of them, where closest match is defined as the longest matching prefix of locality tiers. In a simple, single-tier locality taxonomy using the key "region" this should yield the same behavior as the previous implementation, as all instances with a matching "region" will have the same longest matching prefix (at length 1), however this more general approach should better handle other locality taxonomies that use more tiers and/or tiers with names other than "region". Currently this change only applies to physical planning for secondary tenants until physical planning is unified for system and secondary tenants. Release note: none. Epic: none.
dt
left a comment
There was a problem hiding this comment.
TFTRs!
bors r+
Reviewable status:
pkg/roachpb/metadata.go line 615 at r1 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: something sounds off in "those the passed locality", like a preposition "of" is missing or something.
Done.
Yep, indeed was supposed to say "of the passed..."
pkg/sql/distsql_physical_planner.go line 1381 at r2 (raw file):
Previously, yuzefovich (Yahor Yuzefovich) wrote…
nit: this comment needs a minor adjustment now, probably just
s/region/locality/g.
Done
|
Build failed (retrying...): |
mgartner
left a comment
There was a problem hiding this comment.
💯
Reviewed 1 of 2 files at r1, 4 of 4 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status:
|
Build succeeded: |
This changes physical planning, specifically how the SQL instance for a
given KV node ID is resolved, to be more generalized w.r.t. different
locality tier taxonomies.
Previously this function had a special case that checked for, and only
for, a specific locality tier with the key "region" and if it was
found, picked a random instance from the subset of instances where their
value for that matched the value for the KV node.
Matching on and only on the "region" tier is both too specific and not
specific enough: it is "too specific" in that it requires a tier with
the key "region" to be used and to match, and is "not specific enough"
in that it simultaneously ignores more specific locality tiers that
would indicate closer matches (e.g. subregion, AZ, data-center or rack).
Instead, this change generalizes this function to identify the subset of
instances that have the "closest match" in localities to the KV node and
pick one of them, where closest match is defined as the longest matching
prefix of locality tiers. In a simple, single-tier locality taxonomy
using the key "region" this should yield the same behavior as the
previous implementation, as all instances with a matching "region" will
have the same longest matching prefix (at length 1), however this more
general approach should better handle other locality taxonomies that use
more tiers and/or tiers with names other than "region".
Currently this change only applies to physical planning for secondary
tenants until physical planning is unified for system and secondary
tenants.
Release note: none.
Epic: CRDB-16910