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: deprecate database and index fields on HotRanges proto #137043

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

angles-n-daemons
Copy link
Contributor

@angles-n-daemons angles-n-daemons commented Dec 9, 2024

server: deprecate database and index fields on HotRanges proto

In changeset #134106, the database, table, and index fields of the HotRanges protobuf were moved to synonymous keys so that there could be multiple of each. This means a hot range could now have multiple indexes, tables and databases contained within it in the UI.

During this change however, only the old table key of the protobuf was deprecated, the database and index fields were not flagged for deprecation. This small change fixes this oversight.

Epic: CRDB-43151
Release note: None

@angles-n-daemons angles-n-daemons requested review from a team as code owners December 9, 2024 21:20
@angles-n-daemons angles-n-daemons requested review from dhartunian and removed request for a team December 9, 2024 21:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@@ -1484,9 +1484,9 @@ message HotRangesResponseV2 {
// table_name has been deprecated in favor of tables = 16;
string table_name = 4 [deprecated = true];
// database_name has been deprecated in favor of databases = 17;
string database_name = 5;
string database_name = 5 [deprecated = true];
Copy link
Member

Choose a reason for hiding this comment

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

I think typically we opt for reserved - is there a reason we're doing a softer deprecation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason other than muscle memory - I'll change this.

@angles-n-daemons angles-n-daemons force-pushed the hot-ranges-proto-deprecate-db-index branch 4 times, most recently from a8f65bb to 9fb2ee0 Compare December 17, 2024 22:34
Copy link

blathers-crl bot commented Dec 17, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@angles-n-daemons angles-n-daemons force-pushed the hot-ranges-proto-deprecate-db-index branch from 9fb2ee0 to d00c8c9 Compare December 18, 2024 22:18
@dhartunian dhartunian removed their request for review December 18, 2024 23:09
@angles-n-daemons angles-n-daemons force-pushed the hot-ranges-proto-deprecate-db-index branch from d00c8c9 to 421b874 Compare December 19, 2024 17:55
@angles-n-daemons angles-n-daemons requested a review from a team as a code owner December 19, 2024 17:55
@angles-n-daemons angles-n-daemons requested review from arjunmahishi and aa-joshi and removed request for a team December 19, 2024 17:55
@angles-n-daemons angles-n-daemons force-pushed the hot-ranges-proto-deprecate-db-index branch from 421b874 to bfebae3 Compare December 19, 2024 18:26
In changeset cockroachdb#134106, the database, table, and index fields of the
HotRanges protobuf were moved to synonymous keys so that there could be
multiple of each. This means a hot range could now have multiple
indexes, tables and databases contained within it in the UI.

During this change however, only the old table key of the protobuf was
deprecated, the database and index fields were not flagged for
deprecation. This small change fixes this oversight.

Release note: None
@angles-n-daemons angles-n-daemons force-pushed the hot-ranges-proto-deprecate-db-index branch from bfebae3 to ca76f7d Compare December 19, 2024 19:40
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.

3 participants