sql: fix locality information in show ranges #43807
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @darinpp)
pkg/sql/crdb_internal.go, line 2011 at r1 (raw file):
} voterReplicas := append(
nit: I'd keep this on the same line. It gets a lot louder when spread over 4.
pkg/sql/crdb_internal.go, line 2015 at r1 (raw file):
desc.Replicas().Voters()..., ) var learnerReplicas []int
rename this to learnerReplicaStoreIDs
pkg/sql/crdb_internal_test.go, line 38 at r1 (raw file):
) func TestRangeLocalityBasedOnNodeIDs(t *testing.T) {
Add a comment about what this is testing.
pkg/sql/crdb_internal_test.go, line 64 at r1 (raw file):
) //Set to 1 so the next store id will be 2
Missing space at the beginning of this comment. Also, comments that are sentences should have periods at the end.
pkg/sql/crdb_internal_test.go, line 67 at r1 (raw file):
tc.Servers[0].DB().Put(ctx, keys.StoreIDGenerator, 1) // NodeID=3, StoreID=2
Is there a crdb_internal table we can read to test that this is true?
darinpp
force-pushed
the
fix_range_locality_info
branch
from
e0c9aa9
to
717a5e1
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/crdb_internal.go, line 2011 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
nit: I'd keep this on the same line. It gets a lot louder when spread over 4.
moves to the same line.
pkg/sql/crdb_internal.go, line 2015 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
rename this to
learnerReplicaStoreIDs
renamed
pkg/sql/crdb_internal_test.go, line 38 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Add a comment about what this is testing.
added
pkg/sql/crdb_internal_test.go, line 64 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Missing space at the beginning of this comment. Also, comments that are sentences should have periods at the end.
added space and period
pkg/sql/crdb_internal_test.go, line 67 at r1 (raw file):
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Is there a crdb_internal table we can read to test that this is true?
added assertions for the store ids
darinpp
force-pushed
the
fix_range_locality_info
branch
from
717a5e1
to
4579a24
Compare
There was a problem hiding this comment.
Reviewed 2 of 2 files at r2.
Reviewable status:
Previously the locality information in show ranges was incorrectly assuming that range descriptors are node id based. This was causing a mismatch between replicas tuple and the locality. To address this, this change makes the locality info be based on store ids. Fixes cockroachdb#43755, cockroachdb#30940 Release note (sql change): Show ranges now shows locality information consistent with the range descriptor when node id and store id don't match..
darinpp
force-pushed
the
fix_range_locality_info
branch
from
4579a24
to
2bd8221
Compare
|
bors r+ |
43807: sql: fix locality information in show ranges r=darinpp a=darinpp Previously the locality information in show ranges was incorrectly assuming that range descriptors are node id based. This was causing a mismatch between replicas tuple and the locality. To address this, this change makes the locality info be based on store ids. Fixes #43755, #30940 Release note (sql change): Show ranges now shows locality information consistent with the range descriptor when node id and store id don't match.. 43822: colexec: refactor allocator to reduce the number of function calls r=yuzefovich a=yuzefovich **colexec: refactor allocator to reduce the number of function calls** Previously, allocator had methods to Append or Copy a single coldata.Vec. Those methods created an anonymous function on every call. However, calls to these methods often occur in a loop over many columns, so they become relatively expensive. Now this code has been refactored to Append or Copy all columns at once. Allocator.Append and Allocator.Copy have been removed and Allocator.PerformOperation has been exposed as a single method to use. Release note: None **colexec: minor refactor of memory accounting** We extract out PerformOperation call outside of the loop in Case operator (which should give minor performance improvement in case of multiple When arms) and do a tweak to rowstovec template. Also, a bug with memory accounting in any_not_null template is fixed. Release note: None Co-authored-by: Darin <darinp@gmail.com> Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Build succeeded |
Previously the locality information in show ranges was incorrectly assuming
that range descriptors are node id based.
This was causing a mismatch between replicas tuple and the locality.
To address this, this change makes the locality info be based on store ids.
Fixes #43755, #30940
Release note (sql change): Show ranges now shows locality information
consistent with the range descriptor when node id and store id don't match..