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,workload: don't hide useful workloads #93992

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 20, 2022

Fixes #94079.
Prior to this patch, at least half of the workloads were hidden from view in the output of cockroach --help.

There was no good reason for this: most of the workloads are useful for teaching/learning and for experimentation. They all deserve more exposure, so that folk can learn about them without being told by the one person who built the workload in the first place.

So this patch fixes that by exposing of all of them through the online help.

One question that could remain is how much teaching value there is in letting someone experiment with a tool that was built for the benefit of one team only. One specific workload is under consideration here: bulkingest, used for benchmarking inside the D&R team, does not really do anything akin to what an end-user would possibly expect to do with a database. For that workload, and the benefit of future workloas akin to it, this patch adds a notice in its help text that it was developed for internal testing only.

Release note: None
Epic: None

@knz knz requested a review from j82w December 20, 2022 17:31
@knz knz requested a review from a team as a code owner December 20, 2022 17:31
@knz knz requested a review from a team December 20, 2022 17:31
@knz knz requested review from a team as code owners December 20, 2022 17:31
@knz knz requested review from herkolategan and renatolabs and removed request for a team December 20, 2022 17:31
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor Author

knz commented Dec 20, 2022

@j82w I'm nominating you to review this because we don't have any one team responsible for this part of our infrastructure, and your opinion is thus at least as good as anyone else. I even think your opinion is actually better since you've recently created a workload that deserves this treatment.

@knz knz force-pushed the 20221220-workloads branch 2 times, most recently from bbf7fa1 to ab3826a Compare December 20, 2022 18:14
Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

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

I agree that making them public could be helpful for users. I don't see an issue with it. Not sure if any of the other teams have a reason for keeping them hidden. The plan was to make insights workload public, but wanted to get feedback on the initial implementation before doing so.

Reviewed 16 of 16 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, and @renatolabs)


pkg/workload/ttllogger/ttllogger.go line 50 at r1 (raw file):

	Name:        "ttllogger",
	Description: "Generates a simple log table with rows expiring after the given TTL.",
	Version:     "0.0.1",

Should this be updated to 1.0.0 to match most of the other versions since it will be public? Same thing on ttlbench.go

Copy link
Contributor Author

@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.

TFYR!

bors r=j82w

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @j82w, and @renatolabs)


pkg/workload/ttllogger/ttllogger.go line 50 at r1 (raw file):

Previously, j82w wrote…

Should this be updated to 1.0.0 to match most of the other versions since it will be public? Same thing on ttlbench.go

I think we should trust the teams who authored these tests to maintain the version numbers themselves. This particular value doesn't hurt.

@knz
Copy link
Contributor Author

knz commented Dec 20, 2022

bors r-

@craig
Copy link
Contributor

craig bot commented Dec 20, 2022

Canceled.

Prior to this patch, at least half of the workloads were hidden from
view in the output of `cockroach --help`.

There was no good reason for this: most of the workloads are useful
for teaching/learning and for experimentation. They all deserve more
exposure, so that folk can learn about them without being told by the
one person who built the workload in the first place.

So this patch fixes that by exposing of all of them through the online
help.

One question that could remain is how much teaching value there is in
letting someone experiment with a tool that was built for the benefit
of one team only. One specific workload is under consideration here:
`bulkingest`, used for benchmarking inside the D&R team, does not
really do anything akin to what an end-user would possibly expect to
do with a database. For that workload, and the benefit of future
workloas akin to it, this patch adds a notice in its help text that it
was developed for internal testing only.

Release note: None
@knz knz force-pushed the 20221220-workloads branch from ab3826a to df0ef24 Compare December 20, 2022 22:04
@knz
Copy link
Contributor Author

knz commented Dec 20, 2022

bors r=j82w

Copy link
Contributor

@j82w j82w left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan and @renatolabs)

@craig
Copy link
Contributor

craig bot commented Dec 20, 2022

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.

cli: cockroach demo --help does not list half of the available workloads.
3 participants