ui, server: modify hot ranges api and table to use new contents approx. #134106

angles-n-daemons · 2024-11-01T20:34:23Z

ui, server: modify hot ranges api and table to use new contents approx.

This change is the last in a set of commits to change the hot ranges
page from only showing one table, index per range to many. It builds on
top of the changes in the catalog reader
(10b9ee0) and the range utilities
(109219d) to surface a set of tables,
indexes for each range.

The primary changes in this commit specifically are the modification of
the status server to use the new rangeutil utilities, and changing the
wire, presentation format of the information.

Epic: CRDB-43151
Fixes: #130997

Release note: changes the table, index contents of the hot ranges page in DB console.

blathers-crl · 2024-11-01T20:34:26Z

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2024-11-01T20:34:40Z

This change is

xinhaoz · 2024-11-05T15:25:42Z

pkg/server/apiutil/indexnames.go

nit: can we rename to index_names.go

xinhaoz · 2024-11-05T15:27:32Z

pkg/server/apiutil/indexnames.go

+type IndexNamesList []IndexNames
+
+// ToOutput is a simple function which returns a set of de-duplicated,
+// fully referenced database, table, and index names depending on the,


nit: extra comma end of line

Would it be useful at all to return these values sorted?

Right now they will be sorted, but by how they appear in the range. Would sorting them by name look cleaner?

xinhaoz · 2024-11-05T15:37:50Z

pkg/server/apiutil/indexnames.go

+	for _, idx := range idxl {
+		table := idx.Table
+		if multipleDatabases {
+			table = idx.Database + "." + table


Are these strings quoted? Will we have any issues with db names and tables with . in them?
E.g. database my.db has table table and databas my has table db.table. Is it important that this output differentiates those?

~~Have we run into this issue before? It can't hurt to quote them I suppose, though if it's not necessary I'd prefer against it.~~

Actually, I think this makes sense, I've added quotes if the name contains a ..

xinhaoz · 2024-11-05T15:39:07Z

pkg/server/apiutil/rangeutil.go

-	return idx.Database == other.Database &&
-		idx.Table == other.Table &&
-		idx.Index == other.Index
+// a poor boy wants for a range slice


nit: please merge this asap

oh whoops, i'll remove this.

xinhaoz · 2024-11-05T15:40:18Z

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx

@@ -44,6 +44,19 @@ interface HotRangesTableProps {
  onSortChange?: (ss: SortSetting) => void;
 }

+const tableToCell = (val: any) => {
+  // make an assumption if we see a single table


nit: clean up comment

removed this function since the link isn't quite possible right now.

xinhaoz · 2024-11-05T15:44:45Z

pkg/ui/workspaces/db-console/src/views/hotRanges/hotRangesTable.tsx

+  // make an assumption if we see a single table
+  if (val.tables.length === 1) {
+    return (
+      <Link to={util.EncodeDatabaseTableUri(val.databases[0], val.tables[0])}>


This link looks like it's to the legacy page. For the new pages we use the table id. I guess that might make it difficult to perform this linking since the hot ranges don't seem to return any ids.

~~Gotcha, I'll see if there's a way to link it to the new databases page if possible~~

Ah I see, let me check with Kevin on this one.

Checked in, it's okay for the link to be omitted as part of this release.

Previously each range correlated to a single table, or even a single index in a database, so all that was required to identify which tables, indexes were in the range were to look at the start key of the range and map it accordingly. With range coalescing however, it's possible for one, or many, tables, indexes and the like to reside within the same range. To properly identify the contents of a range, this PR adds the following utilities: 1. A utility function which turns a range into a span, and clamps it to its tenant's table space. 2. A utility function which takes the above spans and uses the catalog and new descriptor by span utility to turn those spans into a set of table descriptors ordered by id. 3. A utility function which transforms those table descriptors into a set of (database, table, index) names which deduplicate and identify each index uniquely. 4. A utility function, which merges the ranges and indexes into a map keyed by RangeID whose values are the above index names. 5. A primary entrypoint for consumers from which a set of ranges can be passed in and a mapping from those ranges to indexes can be returned. A variety of cavets come with this approach. It attempts to scan the desciptors all at once, but it still will scan a sizable portion of the descriptors table if the request is large enough. This makes no attempt to describe system information which does not have a descriptor. It will describe system tables which appear in the descriptors table, but it will not try to explain "tables" which do not have descriptors (example tsdb), or any other information stored in the keyspace without a descriptor (PseudoTableIDs, GossipKeys for example). Throughout this work, many existing utilities were duplicated, and then un-duplicated (`keys.TableDataMin`, `roachpb.Span.Overlap`, etc). If you see anything that seems to already exist, feel free to point it out accordingly. Epic: none Fixes: cockroachdb#130997 Release note: None

kyle-a-wong

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @angles-n-daemons, @lassenordahl, and @xinhaoz)

pkg/roachpb/data.go line 227 at r6 (raw file):

// Clamp fixes the key to something within the range a < k < b.
func (k Key) Clamp(a, b Key) Key {

nit:Can we rename these to min and max? Also do you think we should add validation / enforce that min is <= max?

Suggestion:

min, max Key

pkg/server/apiutil/rangeutil.go line 54 at r6 (raw file):

// GetRangeIndexMappings translates a set of ordered ranges into a
// RangeID -> []IndexNames mapping. It does this by executing the fololowing steps:

nit:sp

Suggestion:

following

pkg/server/apiutil/rangeutil.go line 119 at r6 (raw file):

}

// RangeToTableSpans is a simple utility function which converts a set of ranges

nit: rm

pkg/server/apiutil/rangeutil.go line 162 at r6 (raw file):

}

// TableDescriptorsToIndexNames is a simple function which maps a set of descriptors to the

nit: rm

pkg/server/apiutil/index_names.go line 34 at r9 (raw file):

// toFourPartIdentifier adds quotes to an identifier if it contains
// the four part delimiter '.'.
func toFourPartIdentifier(identifier string) string {

nit: I feel like it would be better to name this "maybeQuoteIdentifier" or something closer to what the function is doing

Code quote:

toFourPartIdentifier

pkg/server/apiutil/index_names.go line 36 at r9 (raw file):

func toFourPartIdentifier(identifier string) string {
	if strings.Contains(identifier, ".") {
		return "\"" + identifier + "\""

super nit: i find it more readable to use back tickets if i want to include "

Suggestion:

return `"` + identifier + `"`

pkg/sql/catalog/descs/collection.go line 1152 at r6 (raw file):

}

// but as a map with the database ID as the key.

I think this should be with the comment of the previous function, and this function needs its old docstring

Code quote:

// but as a map with the database ID as the key.

angles-n-daemons

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @kyle-a-wong, @lassenordahl, and @xinhaoz)

pkg/roachpb/data.go line 227 at r6 (raw file):