cluster-ui, db-console: fix bug where store ids are treated as node ids #119260

laurenbarker · 2024-02-15T20:43:39Z

Previously, store ids were being treated as node ids on the databases view. This resulted in "Regions/Nodes" column showing inaccurate information in some cases. Now, store ids are mapped to node ids on the databases view which resolves the issue.

Fixes: #118957

Release note (ui change): Resolves an issue where clusters with multiple stores per node may list inaccurate region/node information in the databases table.

cockroach-teamcity · 2024-02-15T20:43:45Z

All committers have signed the CLA.

cockroach-teamcity · 2024-02-15T20:43:50Z

This change is

laurenbarker · 2024-02-15T20:52:00Z

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts

@@ -147,7 +160,7 @@ const deriveDatabaseTableDetails = (
  const normalizedPrivileges = normalizePrivileges(
    [].concat(...grants.map(grant => grant.privileges)),
  );
-  const nodes = results?.stats.replicaData.nodeIDs || [];
+  const nodes: Nodes = { kind: "node", ids: results?.stats.replicaData.nodeIDs || [] };


The table views appear to already be using node IDs. Please correct me if I'm reading this incorrectly.

laurenbarker · 2024-02-15T20:52:52Z

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx

-// regionA(n1, n2), regionB(n2,n3), ...
+/** Store ids and node ids are both of type `number[]`. To disambiguate, a
+ * `kind` field is included in the type. */
+export type Stores = { kind: "store", ids: number[] };


Expanding the type is the only way I could think to disambiguate the data of type number[]... Open to alternatives here.

laurenbarker · 2024-02-15T20:54:16Z

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx

@@ -102,7 +142,7 @@ export function nodesByRegionMapToString(
    .join(", ");
 }

-export function createNodesByRegionMap(
+function createNodesByRegionMap(


nodesByRegionMapToString and createNodesByRegionMap are only used in this file, so I removed the exports.

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx

laurenbarker · 2024-02-15T22:44:17Z

Looks like I need to stub an API response in pkg/ui/workspaces/db-console/src/views/databases/databasesPage/redux.spec.ts. I'll do that tomorrow.

laurenbarker · 2024-02-21T00:00:45Z

Rebased on changes from #118996 was merged.

xinhaoz · 2024-02-21T20:52:16Z

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 181 at r2 (raw file):

  );
  /** List of store IDs for the current cluster. All of the values in the
   * `*replicas` columns correspond to store IDs. */

nit: I think this comment can be deleted now since the field was renamed.

xinhaoz

Thanks so much for taking this on (and taking the time to understand what's going on with db console as a whole :)), just some super small nits but otherwise all good 💯

Reviewed 3 of 7 files at r1, 7 of 9 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @laurenbarker)

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 163 at r1 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

The table views appear to already be using node IDs. Please correct me if I'm reading this incorrectly.

Yeah, I believe all the database page views assumed they were using node ids and store ids.

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 180 at r1 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

I'm assuming this should be node ids and not store ids too.

yup! 👍

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 241 at r2 (raw file):

    /** List of store IDs for the current cluster. All of the values in the
     * `*replicas` columns correspond to store IDs. */

same here with the comment

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx line 145 at r1 (raw file):

Previously, laurenbarker (Lauren Barker) wrote…

nodesByRegionMapToString and createNodesByRegionMap are only used in this file, so I removed the exports.

thank you!

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx line 71 at r2 (raw file):

 * @returns A list of node ids for the cluster.
 */
export function getNodeIdsFromStoreIds(

Can we just make this return the Nodes type directly since all its use cases seem to be creating Nodes?

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 112 at r2 (raw file):

  nodes?: number[];
  /** A list of node statuses so that store ids can be mapped to nodes. */
  nodeStatuses: cockroach.server.status.statuspb.INodeStatus[];

I don't think we need to add this to the props.

xinhaoz · 2024-02-21T21:27:49Z

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 163 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Yeah, I believe all the database page views assumed they were using node ids and store ids.

not store ids**

laurenbarker

TFTR @xinhaoz! I think I need help with the last test -- "loads table data" in pkg/ui/workspaces/db-console/src/views/databases/databaseDetailsPage/redux.spec.ts. nodeStatuses is undefined and I'm not sure how to get that information populated in the store and if I need to modify non-test code as well.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 163 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

not store ids**

Oops, this was a comment from before you made your change. I wasn't sure if replicaData.nodeIDs were node ids or store ids. Turns out they were store ids and you updated the name 😄

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 181 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: I think this comment can be deleted now since the field was renamed.

Good call. Done.

pkg/ui/workspaces/cluster-ui/src/databases/combiners.ts line 241 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

same here with the comment

Done.

pkg/ui/workspaces/cluster-ui/src/databases/util.tsx line 71 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Can we just make this return the Nodes type directly since all its use cases seem to be creating Nodes?

Oh yeah, let's do that!

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx line 112 at r2 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

I don't think we need to add this to the props.

Oops, removed!

Previously, store ids were being treated as node ids on the databases view. This resulted in "Regions/Nodes" column showing inaccurate information in some cases. Now, store ids are mapped to node ids on the databases view which resolves the issue. Also, a log message is skipped during tests so that the output isn't polluted with expected warnings. Fixes: cockroachdb#118957 Release note (ui change): Resolves an issue where clusters with multiple stores per node may list inaccurate region/node information in the databases table.

laurenbarker · 2024-03-05T20:01:00Z

@xinhaoz I think this is ready for another pass! (finally)

laurenbarker · 2024-03-11T15:17:28Z

TFTRs @xinhaoz!

laurenbarker · 2024-03-11T15:17:36Z

bors r+

laurenbarker · 2024-03-11T15:23:45Z

blathers backport 23.2 23.1

blathers-crl · 2024-03-11T15:23:56Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from e3c09d0 to blathers/backport-release-23.1-119260: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1 failed. See errors above.

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

craig · 2024-03-11T16:39:29Z

Build succeeded:

Bazel Essential CI (Cockroach)

laurenbarker force-pushed the store-ids branch 2 times, most recently from fb82441 to b83f754 Compare February 15, 2024 20:50

laurenbarker commented Feb 15, 2024

View reviewed changes

laurenbarker marked this pull request as ready for review February 15, 2024 20:55

laurenbarker requested a review from a team as a code owner February 15, 2024 20:55

laurenbarker requested review from xinhaoz and removed request for a team February 15, 2024 20:55

laurenbarker commented Feb 15, 2024

View reviewed changes

pkg/ui/workspaces/cluster-ui/src/databasesPage/databasesPage.tsx Outdated Show resolved Hide resolved

laurenbarker force-pushed the store-ids branch 2 times, most recently from d74ad6f to 07922e8 Compare February 15, 2024 21:20

laurenbarker force-pushed the store-ids branch 3 times, most recently from 300f898 to f97384b Compare February 20, 2024 23:53

laurenbarker force-pushed the store-ids branch 2 times, most recently from 38b1347 to 68fe962 Compare February 21, 2024 17:26

xinhaoz reviewed Feb 21, 2024

View reviewed changes

laurenbarker force-pushed the store-ids branch 2 times, most recently from 9c24f4a to 3b6f013 Compare February 21, 2024 23:13

laurenbarker commented Feb 21, 2024

View reviewed changes

laurenbarker force-pushed the store-ids branch from 3b6f013 to 94acf91 Compare March 4, 2024 22:07

laurenbarker force-pushed the store-ids branch from 94acf91 to e3c09d0 Compare March 5, 2024 19:02

laurenbarker requested a review from xinhaoz March 5, 2024 20:01

xinhaoz approved these changes Mar 11, 2024

View reviewed changes

blathers-crl bot mentioned this pull request Mar 11, 2024

release-23.2: cluster-ui, db-console: fix bug where store ids are treated as node ids #120212

Merged

craig bot merged commit 436fcd5 into cockroachdb:master Mar 11, 2024
17 of 18 checks passed

laurenbarker mentioned this pull request Mar 11, 2024

release-23.1: cluster-ui, db-console: fix bug where store ids are treated as node ids #120234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster-ui, db-console: fix bug where store ids are treated as node ids #119260

cluster-ui, db-console: fix bug where store ids are treated as node ids #119260

laurenbarker commented Feb 15, 2024

cockroach-teamcity commented Feb 15, 2024 •

edited

cockroach-teamcity commented Feb 15, 2024

laurenbarker Feb 15, 2024

laurenbarker Feb 15, 2024

laurenbarker Feb 15, 2024

laurenbarker commented Feb 15, 2024

laurenbarker commented Feb 21, 2024

xinhaoz commented Feb 21, 2024

xinhaoz left a comment

xinhaoz commented Feb 21, 2024

laurenbarker left a comment

laurenbarker commented Mar 5, 2024

laurenbarker commented Mar 11, 2024

laurenbarker commented Mar 11, 2024

laurenbarker commented Mar 11, 2024

blathers-crl bot commented Mar 11, 2024

craig bot commented Mar 11, 2024

cluster-ui, db-console: fix bug where store ids are treated as node ids #119260

cluster-ui, db-console: fix bug where store ids are treated as node ids #119260

Conversation

laurenbarker commented Feb 15, 2024

cockroach-teamcity commented Feb 15, 2024 • edited

cockroach-teamcity commented Feb 15, 2024

laurenbarker Feb 15, 2024

Choose a reason for hiding this comment

laurenbarker Feb 15, 2024

Choose a reason for hiding this comment

laurenbarker Feb 15, 2024

Choose a reason for hiding this comment

laurenbarker commented Feb 15, 2024

laurenbarker commented Feb 21, 2024

xinhaoz commented Feb 21, 2024

xinhaoz left a comment

Choose a reason for hiding this comment

xinhaoz commented Feb 21, 2024

laurenbarker left a comment

Choose a reason for hiding this comment

laurenbarker commented Mar 5, 2024

laurenbarker commented Mar 11, 2024

laurenbarker commented Mar 11, 2024

laurenbarker commented Mar 11, 2024

blathers-crl bot commented Mar 11, 2024

craig bot commented Mar 11, 2024

cockroach-teamcity commented Feb 15, 2024 •

edited