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

cli/demo: pre-load 'movr' by default #39298

Merged
merged 2 commits into from
Aug 4, 2019

Conversation

knz
Copy link
Contributor

@knz knz commented Aug 3, 2019

Requested/suggested by @kenliu.
cc @piyush-singh

Release note (cli change): cockroach demo without argument becomes
equivalent to cockroach demo movr. The previous behavior
(no pre-defined dataset) is still available via cockroach demo --empty.

@knz knz requested a review from jordanlewis August 3, 2019 19:23
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Release note (cli change): `cockroach demo` without argument becomes
equivalent to `cockroach demo movr`. The previous behavior
(no pre-defined dataset) is still available via `cockroach demo
--empty`.
@knz knz force-pushed the 20190803-demo-defaultdb branch from 0314c59 to 69f1daf Compare August 3, 2019 21:33
Copy link
Member

@jordanlewis jordanlewis left a comment

Choose a reason for hiding this comment

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

LGTM. You might want to add some more text to the intro paragraph of cockroach demo now. It currently says:

#
# Welcome to the CockroachDB demo database!
#
# You are connected to a temporary, in-memory CockroachDB
# cluster of 3 nodes. Your changes will not be saved!
#
# Web UI: http://127.0.0.1:57774
#
# Server version: CockroachDB CCL unknown (darwin amd64, built , go1.12.4) (same version as client)
# Cluster ID: 66a60ca1-d154-48df-bb99-7a779da9d08d
#
# Enter \? for a brief introduction.
#

Perhaps in the first paragraph you could say that it's been preloaded with a fictional ridesharing dataset called movr?

@knz
Copy link
Contributor Author

knz commented Aug 4, 2019

Perhaps in the first paragraph you could say that it's been preloaded with a fictional ridesharing dataset called movr?

Good idea. Done.

TFYR!

bors r+

craig bot pushed a commit that referenced this pull request Aug 4, 2019
39298: cli/demo: pre-load 'movr' by default r=knz a=knz

Requested/suggested by @kenliu.
cc @piyush-singh 

Release note (cli change): `cockroach demo` without argument becomes
equivalent to `cockroach demo movr`. The previous behavior
(no pre-defined dataset) is still available via `cockroach demo
--empty`.

Co-authored-by: Raphael 'kena' Poss <[email protected]>
@craig
Copy link
Contributor

craig bot commented Aug 4, 2019

Build succeeded

@craig craig bot merged commit 6091118 into cockroachdb:master Aug 4, 2019
@knz knz deleted the 20190803-demo-defaultdb branch August 4, 2019 04:01
@kenliu
Copy link

kenliu commented Aug 5, 2019

thanks @knz !

closes #39156

@danhhz
Copy link
Contributor

danhhz commented Aug 6, 2019

Awesome!

@awoods187
Copy link
Contributor

Should we backport this?

@knz
Copy link
Contributor Author

knz commented Aug 6, 2019

It's a feature change, so as per policy... no?

@danhhz
Copy link
Contributor

danhhz commented Aug 6, 2019

For the same reason, the movr workload itself has not been backported. I've asked the docs team to ping me if it ends up being a big pain for them that movr is not in 19.1 (and that I'd then go feel out if we'd be willing to make an exception), but so far I haven't heard anything on that.

@jseldess
Copy link
Contributor

jseldess commented Aug 6, 2019

If we can make an exception on this, I'd love to get the movr workload (and demo movr) into 19.1, given that customers tend to stay on the stable release for a long while before upgrading, and we're starting to leverage movr throughout our SQL docs.

@danhhz
Copy link
Contributor

danhhz commented Aug 6, 2019

Well that's pretty definitive as far as interest goes. @bdarnell are we comfortable with backporting the movr workload impl into 19.1 (#37709 and #38203)? It clearly falls in the "new feature" bucket, but it's hard to imagine where any stability risk would come from, as it's just adding another implementation of the workload.Generator interface. Those PRs do touch a few files outside of pkg/workload/movr, but I could easily skip over those in the backport.

I think it's worth considering this PR separately, given that there's a very easy workaround (cockroach demo movr instead of cockroach demo) and changing the behavior of cockroach demo could potentially break someone's script.

@bdarnell
Copy link
Contributor

bdarnell commented Aug 8, 2019

I don't mind too much because it's essentially zero risk, but it does seem a little late to change this. It's too late for 19.1.4, so this will go into 19.1.5 in September. Is it worth the trouble to get this out for just a month before 19.2?

People don't upgrade often, but that also means that people will be sticking with 19.1.3 so we'll have to be careful in the docs to point out that this will require 19.1.5+. The target audience for this feature would mainly be new users, who will presumably be starting with 19.2 as soon as it's available.

I think it's worth considering this PR separately, given that there's a very easy workaround (cockroach demo movr instead of cockroach demo) and changing the behavior of cockroach demo could potentially break someone's script.

I'm not too concerned about scripting of cockroach demo, and I wouldn't want to introduce cockroach demo movr in 19.1.5 if it just changes back to cockroach demo in 19.2.

@jseldess
Copy link
Contributor

jseldess commented Aug 8, 2019

Good points, @bdarnell. If it's unlikely for users on 19.1 to upgrade to 19.1.5, then this isn't worth it.

@awoods187
Copy link
Contributor

@bdarnell what's your evidence that people won't upgrade to 19.1.5? I think many customers will not join 19.2.0 which means 19.2.1 isn't until November. It seems like this would have value in the meantime. In addition, plenty of customers will continue to use 19.1 even into 19.2.x.

@bdarnell
Copy link
Contributor

bdarnell commented Aug 8, 2019

Just anecdotal; feel free to pull actual data here. But even if people do upgrade quickly to 19.1.5, the marginal value of getting movr-based documentation out there a couple of months early seems fairly small to me.

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.

8 participants