sql: Reorder show ranges output to be clearer #40501
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @rohany and @solongordon)
pkg/sql/show_ranges_test.go, line 43 at r1 (raw file):
replicas := make([]int, 3) q := `SELECT replicas, replica_localities from [SHOW RANGES FROM TABLE t]`
Please make sure the leaseholder locality column is also tested, either here or in a logic test or both.
pkg/sql/delegate/show_ranges.go, line 46 at r1 (raw file):
range_size / 1000000 as range_size_mb, lease_holder, replica_localities[1] as lease_holder_locality,
Wait, is replica_localities[1] really the leaseholder locality? That seems arbitrary and incorrect to me unless I'm missing something.
Also... whitespace. 😛
rohany
force-pushed
the
show-ranges-output
branch
from
24b629d
to
de118fe
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/show_ranges_test.go, line 43 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Please make sure the leaseholder locality column is also tested, either here or in a logic test or both.
done
pkg/sql/delegate/show_ranges.go, line 46 at r1 (raw file):
Previously, solongordon (Solon) wrote…
Wait, is
replica_localities[1]really the leaseholder locality? That seems arbitrary and incorrect to me unless I'm missing something.Also... whitespace. 😛
yeah you're right, I'm wrong here. For some reason I had it in my mind that the leaseholder was the first thing in the replicas list.
rohany
force-pushed
the
show-ranges-output
branch
2 times, most recently
from
493d2ae
to
54eb496
Compare
The output of show ranges had column orderings that at a quick glance could lead to users making the wrong conclusions about their partitioning setup. This PR adjusts the columns and adds more readily accessible information about the lease_holder node's locality. Fixes cockroachdb#40467. Release note (sql change): Reorder columns in show ranges output
rohany
force-pushed
the
show-ranges-output
branch
from
54eb496
to
5f8a9d7
Compare
|
someone fix my goland to do whitespace correctly |
There was a problem hiding this comment.
Reviewable status:
pkg/sql/delegate/show_ranges.go, line 46 at r1 (raw file):
Previously, rohany (Rohan Yadav) wrote…
yeah you're right, I'm wrong here. For some reason I had it in my mind that the leaseholder was the first thing in the replicas list.
Great, makes more sense now.
I think if you convert all the tabs to spaces in these strings the whitespace gods will be appeased.
|
bors r+ |
40501: sql: Reorder show ranges output to be clearer r=rohany a=rohany The output of show ranges had column orderings that at a quick glance could lead to users making the wrong conclusions about their partitioning setup. This PR adjusts the columns and adds more readily accessible information about the lease_holder node's locality. Fixes #40467. Release note (sql change): Reorder columns in show ranges output 40519: workload: ignore syntax error on INJECT STATISTICS against v2.0 r=nvanbenschoten a=nvanbenschoten Fixes #40463. Release note: None Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu> Co-authored-by: Nathan VanBenschoten <nvanbenschoten@gmail.com>
Build succeeded |
The output of show ranges had column orderings that at a quick glance
could lead to users making the wrong conclusions about their
partitioning setup. This PR adjusts the columns and adds more readily
accessible information about the lease_holder node's locality.
Fixes #40467.
Release note (sql change): Reorder columns in show ranges output