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

demo: Add location information to default demo localities. #40229

Merged
merged 1 commit into from
Aug 29, 2019

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Aug 26, 2019

Fixes #39937.

Adds default latitude and longitude information for the default
localities in cockroach demo so that the web UI is pre-populated.

Release note (cli change): Add location information to default
localities to populate web UI for cockroach demo.

@rohany rohany requested review from jordanlewis and a team August 26, 2019 22:49
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rohany
Copy link
Contributor Author

rohany commented Aug 26, 2019

Web UI with just running cockroach demo --nodes 9

image

@rohany rohany changed the title cli: Add location information to default demo localities. demo: Add location information to default demo localities. Aug 26, 2019
@jordanlewis
Copy link
Member

Sweet! I think we could take this one step further and have system.locations get populated by a migration or startup sequence so that every cluster has these default locations. It's pretty silly that we have to ask people to fill it out (and somehow I doubt people currently do). cc @piyush-singh - thoughts?

@knz
Copy link
Contributor

knz commented Aug 27, 2019

👍 on pre-populating system.locations upfront for everyone.

however I could also see wisdom in providing a SQL data file as a separate DLC, with two tiers: one free tier with just 3 locations that's included in the binary (and used for demo) and one downloadable DLC which registers the feature use to marketing or something?

@piyush-singh maybe food for thought

@jordanlewis
Copy link
Member

In the name of reducing friction for developers, and inspiring developer love, I would like to push back against that idea - the data is all free and easily accessible. Let's add it by default and be done with it - I don't want our users to see a semi-working node map and have to figure out how to make it fully-working.

@knz
Copy link
Contributor

knz commented Aug 27, 2019

ok

an unrelated question - are you OK with these region names? ISTM these are identifiers that are specific to GCP, is the mapping between identifier and geographical location the same for EC2 and Azure?

@jordanlewis
Copy link
Member

The mappings are different. Conveniently, all 3 clouds provide different region names :)

@awoods187
Copy link
Contributor

I understand the idea of wanting to help marketing out here but I agree with @jordanlewis that the above proposal creates unnecessary developer friction.

@knz
Copy link
Contributor

knz commented Aug 27, 2019

The mappings are different. Conveniently, all 3 clouds provide different region names :)

oh that's handy 🎉

sure then, it will make tutorials even more spiffy

@rohany
Copy link
Contributor Author

rohany commented Aug 27, 2019

Sweet! I think we could take this one step further and have system.locations get populated by a migration or startup sequence so that every cluster has these default locations. It's pretty silly that we have to ask people to fill it out (and somehow I doubt people currently do). cc @piyush-singh - thoughts?

oh, you want these to be published when starting any cluster, not just the demo? If we are doing that, might as well just prepopulate the system locations table with information about every region for all the cloud providers -- its on our docs anyway (https://www.cockroachlabs.com/docs/v19.1/enable-node-map.html)

@jordanlewis
Copy link
Member

oh, you want these to be published when starting any cluster, not just the demo? If we are doing that, might as well just prepopulate the system locations table with information about every region for all the cloud providers -- its on our docs anyway

Yeah, that's what I was talking about too. I think we should just go ahead and do that - would help a lot of people.

@awoods187
Copy link
Contributor

Yeah i agree--would be great

@rohany
Copy link
Contributor Author

rohany commented Aug 28, 2019

I set up populating the system.locations table as a migration right now, but I'm still looking into how to bake in that information during cluster startup.

As of now I have the migration be a noop if the system.locations table is not empty (to avoid overwriting users data), but we could change that.

PTAL

Fixes cockroachdb#39937.

Adds default latitude and longitude information for the default
localities in cockroach demo so that the web UI is pre-populated.

Release note (cli change): Add location information to default
localities to populate web UI for cockroach demo.
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

nice.

@rohany
Copy link
Contributor Author

rohany commented Aug 29, 2019

bors r=knz

I can put up a PR later with all the GCE localities and locations set up, rather than just the ones used in the demo cluster.

craig bot pushed a commit that referenced this pull request Aug 29, 2019
40229: demo: Add location information to default demo localities. r=knz a=rohany

Fixes #39937.

Adds default latitude and longitude information for the default
localities in cockroach demo so that the web UI is pre-populated.

Release note (cli change): Add location information to default
localities to populate web UI for cockroach demo.

40271: demo: Add a Movr workload to run in cockroach demo r=danhhz,nstewart a=rohany

Fixes #39944.

When the `--with-load` flag is passed to demo, a sample movr
workload is run against the database.

I tried my best to replicate the Movr load in Nate's example script.

Based on talks with Dan, it was better to copy out a simpler
version of the logic to run the workload than try to hook
into workload.

Release note (cli change): Add a workload to run with cockroach demo.

40320: randutil: allow COCKROACH_RANDOM_SEED to seed NewPseudoRand r=mjibson a=mjibson

Multiple people have been confused about the lack of interaction
between NewPseudoRand (which used to always generate a random seed)
and COCKROACH_RANDOM_SEED (which only influenced using the global rand
methods).

Release note: None

Co-authored-by: Rohan Yadav <rohany@alumni.cmu.edu>
Co-authored-by: Matt Jibson <matt.jibson@gmail.com>
@craig
Copy link
Contributor

craig bot commented Aug 29, 2019

Build succeeded

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.

system: add default set of configured system.locations
5 participants