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

sql: system.localities uses a single level of locality #20653

Closed
couchand opened this issue Dec 12, 2017 · 12 comments
Closed

sql: system.localities uses a single level of locality #20653

couchand opened this issue Dec 12, 2017 · 12 comments
Assignees
Labels
A-docs A-webui-nodemap C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-done docs-known-limitation no-issue-activity S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. X-stale

Comments

@couchand
Copy link
Contributor

The initial version of the system.locality table (#19652) stores lat/long information according to a single key/value pair of localities, on the assumption that these are globally unique. If two values on the same key have the same value, but are nested under different parent localities, they will be treated as distinct for most purposes, but they can't both be assigned lat/long data. For instance, if one node in a cluster is started with --locality=region=west,datacenter=1 and another with --locality=region=east,datacenter=1, the system table doesn't support assigning lat/long at the datacenter level.

There seem to be two potential solutions to this issue: validate the locality flags assigned to nodes in the cluster and fail if there's a collision, or expand the system table to track all ancestor key/value pairs, not just a single level.

In the meantime, we should probably document the limitation.

@couchand couchand added docs-todo docs-known-limitation S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. labels Dec 12, 2017
@couchand
Copy link
Contributor Author

The way I see it, it's better to allow this and make sure we support it fully than to validate against it and error out. From a usability perspective we should only add limits like this if they're needed.

@bdarnell
Copy link
Contributor

Does the allocator make the same assumption that these names are globally unique, or does it look at the entire hierarchy?

@a-robinson
Copy link
Contributor

The allocator makes no assumptions about uniqueness.

@vilterp
Copy link
Contributor

vilterp commented Feb 27, 2018

maybe we can put this in the docs cc @Amruta-Ranade

An example of what you can't do

  1. Set up locality flags like this:
region=us-east-1
  az=a
region=eu-west-1
  az=a
  1. Try to add locations to both az=a's:
    Primary key uniqueness violation on insert to system.locations, since the primary key is (localityKey, localityValue).

What you would do instead:

  1. Set up your locality flags the same way:
region=us-east-1
  az=a
region=eu-west-1
  az=a
  1. Add locations at the region level, since az's within a region are effectively in the same place anyway.

@couchand
Copy link
Contributor Author

couchand commented Mar 5, 2018

I would edit the "what you would do instead" to suggest renaming the az values to be unique, rather than moving the locations up a level, so the solution handles the case where there are actually several child localities.

@vilterp
Copy link
Contributor

vilterp commented Mar 5, 2018

True, could also do

region=us-east-1
  az=us-east-1a
region=eu-west-1
  az=eu-west-1a

@vilterp
Copy link
Contributor

vilterp commented Mar 14, 2018

@Amruta-Ranade if you've sufficiently documented this limitation, maybe we should close this out or move it to 2.1, since we don't have a plan to address this before 2.0.

@Amruta-Ranade Amruta-Ranade modified the milestones: 2.0, 2.1 Mar 20, 2018
@Amruta-Ranade
Copy link
Contributor

Moving it to 2.1 (documented as discussed for 2.0)

@jseldess
Copy link
Contributor

jseldess commented Apr 1, 2018

Added to 2.0 known limitations in cockroachdb/docs#2823.

@jseldess jseldess reopened this Apr 11, 2018
@knz knz added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Apr 27, 2018
@knz knz added the A-docs label Apr 27, 2018
@couchand couchand modified the milestones: 2.1, 2.2 Sep 18, 2018
@petermattis petermattis removed this from the 2.2 milestone Oct 5, 2018
@sploiselle
Copy link
Contributor

@vilterp @couchand Is this still a known limitation?

@couchand
Copy link
Contributor Author

@sploiselle yep, no change.

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
5 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs A-webui-nodemap C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) docs-done docs-known-limitation no-issue-activity S-3-ux-surprise Issue leaves users wondering whether CRDB is behaving properly. Likely to hurt reputation/adoption. X-stale
Projects
None yet
Development

No branches or pull requests

9 participants