Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

server: add NodeIds to DatabaseDetails and TableStats responses #69788

Merged
merged 1 commit into from
Sep 3, 2021

Conversation

xinhaoz
Copy link
Member

@xinhaoz xinhaoz commented Sep 2, 2021

Related to: #63391

Previously, DatabaseDetails and TableStats did not return a list
of nodes that store their data. This commit adds an ordered
list of node ids to each of the responses. This is necessary
to report node/region information in the DB console databases
pages. For the database details endpoint, we return the list
of nodes only if stats are requested.

Release justification: low risk, high benefit changes to existing
functionality

Release note (api change): A list of node ids representing the
nodes that store data for the database has been added to the
stats field in the databases details endpoint under nodeIds.
Database details must be requested with include_stats set to true,
e.g. /_admin/v1/databases/{database}?include_stats=true

Similarly, nodeIds has also been added to the table stats
endpoint, which is an ordered list of node ids which stores
the table data:
/_admin/v1/databases/{database}/tables/{table}/stats

@xinhaoz xinhaoz requested a review from a team as a code owner September 2, 2021 19:52
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@xinhaoz xinhaoz requested a review from a team September 2, 2021 19:52
@Azhng
Copy link
Contributor

Azhng commented Sep 2, 2021

More of a general comment. The concept of NodeID is going away in the Serverless. The tenant nodes will only be able to have SQLInstanceID assigned to the SQL Pod, since all the actual nodes (the KV Pods in this case) are shared among all the tenants.

We already have a handful of RPC endpoints that need to deal with this awkwardness. Should even further pollute the NodeID concept into more RPC endpoints?

@maryliag
Copy link
Contributor

maryliag commented Sep 2, 2021

This information will not be displayed on serverless, precisely because nodes don't make sense on it. But we need this for dedicated and specially multi-region, and on those cases the value is indeed node. So for the purpose of this issue, use NodeID.

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 4 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @xinhaoz)


pkg/server/admin.go, line 560 at r1 (raw file):

	}

	// Track all nodes storing database

nit: databases
nit: period

Copy link
Contributor

@maryliag maryliag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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

Related to: cockroachdb#63391

Previously, DatabaseDetails and TableStats did not return a list
of nodes that store their data. This commit adds an ordered
list of node ids to each of the responses. This is necessary
to report node/region information in the DB console databases
pages. For the database details endpoint, we return the list
of nodes only if stats are requested.

Release justification: low risk, high benefit changes to existing
functionality

Release note (api change): A list of node ids representing the
nodes that store data for the database has been added to the
stats field in the databases details endpoint under `nodeIds`.
Database details must be requested with include_stats set to true,
e.g. `/_admin/v1/databases/{database}?include_stats=true`

Similarly, `nodeIds` has also been added to the table stats
endpoint, which is an ordered list of node ids which stores
the table data:
`/_admin/v1/databases/{database}/tables/{table}/stats`
@xinhaoz
Copy link
Member Author

xinhaoz commented Sep 2, 2021

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 3, 2021

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants