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

clusterviz: assign lat / long to nodes in the map view #19532

Closed
dianasaur323 opened this issue Oct 25, 2017 · 12 comments
Closed

clusterviz: assign lat / long to nodes in the map view #19532

dianasaur323 opened this issue Oct 25, 2017 · 12 comments
Milestone

Comments

@dianasaur323
Copy link
Contributor

dianasaur323 commented Oct 25, 2017

We have to do some digging to figure out the different options where we can assign a lat / long to a node or locality in the map.

Take for example something that looks like the following:

----------------- [ Country 1 ] ------------ [ Country 2 ] ------------------
-----------------/-----------|--------------/-------------|----------------
---------- [ Region 1 ] --- [ Region 2 ] -- [ Region 3] -- [Region 4] --------
----------/------|--------/-------|-------/-------|------/--------|--------
-----[ DC1 ] - [ DC2] - [DC3] - [DC4] - [DC5] - [DC6] - [DC7] - [DC8]----

First off, I think we don't have to support the state when not all localities within a locality tier have lat / long set. We need to come up with an error message here. We also don't need to support the state when for example, locality level 1 is not set, but locality 2 is set. We should require that at the very least, the parent must have a locality set. If the child does not have a locality set in this tree, we can default to a non-map view.

For now, although the usability will leave much to be desired, let's figure out how to do this with flags. This means we need to attribute a lat/long to a locality, as well as a lat/long potentially to a store attribute.

Eventually, we might consider allowing users to set this in the admin UI, but let's see what this looks like first?

cc @kuanluo @cuongdo @couchand @vilterp @mrtracy for comments

@dianasaur323 dianasaur323 added this to the 1.2 milestone Oct 25, 2017
@bdarnell
Copy link
Contributor

We could also include hard-coded location maps for certain popular naming conventions (names like us-east, country codes, airport codes, etc) and follow those conventions in our docs for the --locality flag. This would give us coverage for a significant fraction of deployments without giving our users extra manual work or defining an input method.

@cuongdo
Copy link
Contributor

cuongdo commented Oct 25, 2017

Spencer also mentioned adding command line parameters to cockroach start for specifying lat/long

@couchand
Copy link
Contributor

Ugh, this UX doesn't sound very nice. Our command-line flags are inscrutable as is, just including things that actually impact the operation of the database. Asking the user to stop nodes so they can add lat/long flags, documenting these flags, dealing with conflicts, etc. all just seems like creating problems for ourselves.

Until there's a GUI flow, I'm inclined to go with Ben's suggestion, pick a few standard lists to leverage, and if a user's localities don't match, they just don't see the map.

@dianasaur323
Copy link
Contributor Author

dianasaur323 commented Oct 25, 2017

I agree, the UX is going to be terrible. The concern I have around standard lists is what if people use slightly different syntax to define those lists? What if us-east is actually us-east virginia not us-east (the other one)?

@dianasaur323
Copy link
Contributor Author

cc @spencerkimball could you provide some more detail here?

@kuanluo
Copy link

kuanluo commented Oct 26, 2017

+1 to @bdarnell's suggestion for the immediate release. Unless the UX to set up lat/long is seamless, it's going to be much harder for people to see and utilize the map. We can perhaps include some obvious syntax pattern in the list to start, and go from there.

@spencerkimball
Copy link
Member

Either 1) command line flag, which is now we set other node-specific items right now. Benefit to this is simplicity. Problem with it is excess redundancy. 2) settings table fields which map locality prefix to latitude longitude pair. 3) an internal system table mapping locality prefix to latitude and longitude.

I think 3) is over engineered and 1) probably redundant and burdensome. So 2) seems like the best option.

@spencerkimball
Copy link
Member

@mberhault could you weigh in on the preferred option here?

@dt
Copy link
Member

dt commented Oct 26, 2017

I don't know if i'd put the locality -> geo mapping in cluster settings: it doesn't really need to be preemptively gossiped to all nodes and kept in memory at all times.

My initial inclination would be system.locality_geo table. I think will also be easier to interact with, since with cluster settings you'd either need to bundle/unbundle in some big proto or add lots of individual settings, which i'm hesitant to do especially if we're shipping default mappings for all the common ones.

@mberhault
Copy link
Contributor

mberhault commented Oct 26, 2017

I would tend to say definitely not for 1), too unwieldy. If 2) means the existing cluster settings then probably not, those are meant to be simple single-value variables and managed completely differently.

  1. would be preferable, but there are two issues regardless of the method used:
  • pre-populating it with reasonable values (eg: most regions for most cloud providers, and we can't be as simple as saying us-east, as that will be in different places depending on which cloud you're on).
  • easy interface to add new entries (something like "click on the map and enter a pattern to match", or "select a locality with unknown location and click on the map to set it". Either way we need admin-UI login)

@spencerkimball
Copy link
Member

Sounds like a system table is preferred.

I don’t think we should prepopulate the table. That’s too prescriptive and it’s confusing. We should maintain a table of common values in the documentation however.

I also don’t think the ease of setting requires any extra work until we find out from customers that they are for some reason unable look up coordinates on their own.

@dianasaur323
Copy link
Contributor Author

@dt did mention that it wouldn't "really" be the end of the world to let users click a point on a map that updates the system table, since it doesn't really compromise anything, but I have very little context on this

spencerkimball added a commit to spencerkimball/cockroach that referenced this issue Oct 30, 2017
Adds a new table, `system.locations`, which maps a string locality
(i.e. "city=New York City") to geographic coordinates, specified as
a latitude, longitude decimal pair.

A new admin API call returns the contents of the table, for use from
the admin UI.

Fixes cockroachdb#19532
spencerkimball added a commit to spencerkimball/cockroach that referenced this issue Dec 11, 2017
Adds a new table, `system.locations`, which maps a string locality
(i.e. "city=New York City") to geographic coordinates, specified as
a latitude, longitude decimal pair, specified in degrees.

A new admin API call returns the contents of the table, for use from
the admin UI.

Fixes cockroachdb#19532

Release note (ui): A system table to allow operators to designate
geographic coordinates for any locality. For use with frontend
cluster visualization.
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

No branches or pull requests

8 participants