cockroachdb / cockroachdb/cockroach
server,ua: add support for `--locality-advertise-addr` for sql-instances
- Dominant language
- Go
- Stars
- 32.5k
- Forks
- 4.1k
- PR merge metrics
- PR metrics pending
Description
This is a prerequisite for merging PR https://github.com/cockroachdb/cockroach/pull/130235.
In the above PR, we change the `sqlInstanceDialer` to use address resolver based on the information from `system.sql_instances` table for both system and non-system tenants, as opposed to current method which uses gossip information based resolver for the system tenant.
However, as mentioned in https://github.com/cockroachdb/cockroach/pull/130235#issuecomment-2376034189, the current gossip based `kvNodeDialer` uses `LocalityAddress` (`--locality-advertise-addr`) and defaults to the `Address` (`--advertise-addr`) of the node (descriptor). But the SQL instance based address resolver only have the `Address` field available. This could potentially degrade performance in certain cases.
https://github.com/cockroachdb/cockroach/blob/8f00a5b02724c1302d77fa32993b17c2b7873750/pkg/gossip/gossip.go#L876-L888
We should add the support for locality-aware advertised address (`--locality-advertise-addr`) in the `system.sql_instances` table and use that in the address resolver to bridge this gap.
This old PR has an example of how to add a field to a system table: https://github.com/cockroachdb/cockroach/pull/128283
Jira issue: CRDB-46312
Contributor guide
Assessment
This issue has not been assessed yet.