cockroachdb / cockroachdb/cockroach

server,ua: add support for `--locality-advertise-addr` for sql-instances

Open
#138,715 3 comments 0 reactions 1 assignee Claimed by @Nukitt View on GitHub
C-enhancement P-3 T-db-server
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.