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: use seeded random for schemachange opsgen #108922

Merged
merged 1 commit into from
Aug 22, 2023

Conversation

annrpom
Copy link
Contributor

@annrpom annrpom commented Aug 17, 2023

A schemachange TestWorkload failure was difficult to reproduce; to address this, this patch uses randutil's NewTestRand() to allow for a global seed to be set when stressing this test.

Epic: none
Informs: #108695
Informs: #105517
Informs: #109218
Release note: none

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@annrpom annrpom requested a review from rafiss August 17, 2023 18:50
@annrpom annrpom marked this pull request as ready for review August 17, 2023 18:50
@annrpom annrpom requested a review from a team as a code owner August 17, 2023 18:50
@annrpom annrpom requested review from herkolategan and smg260 and removed request for a team August 17, 2023 18:50
@annrpom
Copy link
Contributor Author

annrpom commented Aug 17, 2023

The failure originated from branch 23.1, but was last seen on 23.1.9-rc - we should backport to the branch that has seen the most recent failure. Not sure if we want to backport to 23.1 as well

@renatolabs
Copy link
Contributor

This is great, I remember wanting this in the past (for reproducing test failures).

If we're changing this code, it might be a good time to also support passing the seed as a command line argument. Lots of workloads already do it so it would be more consistent.

There's some shared code in workload/random.go that we ca use [1]. See the bank workload [2] for an example of it being used.

[1]

type RandomSeed interface {
LogMessage() string
}

[2]

var RandomSeed = workload.NewUint64RandomSeed()

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm, just need to fix the release note!

also, in the PR description can you link to the test that prompted this. add a line like:

informs https://github.com/cockroachdb/cockroach/issues/108695
informs https://github.com/cockroachdb/cockroach/issues/105517

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


-- commits line 11 at r1:
nit: this should have Release note: None since this is not a change that end-users need to know about.

@renatolabs
Copy link
Contributor

(Forgot to mention my comment is totally optional; that can be done in the future)

@annrpom
Copy link
Contributor Author

annrpom commented Aug 18, 2023

thank you for the suggestion, @renatolabs i'll keep that in mind :]

A schemachange TestWorkload failure was difficult to
reproduce; to address this, this patch uses randutil's
`NewTestRand()` to allow for a global seed to be set when
stressing this test.

Epic: none

Release note: none
@annrpom annrpom requested a review from rafiss August 18, 2023 14:09
@rafiss rafiss added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Aug 22, 2023
@annrpom
Copy link
Contributor Author

annrpom commented Aug 22, 2023

TFTR! ('-')7

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 22, 2023

Build succeeded:

@craig craig bot merged commit a03be32 into cockroachdb:master Aug 22, 2023
@blathers-crl
Copy link

blathers-crl bot commented Aug 22, 2023

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from b21b3e5 to blathers/backport-release-23.1-108922: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.x failed. See errors above.


error creating merge commit from b21b3e5 to blathers/backport-release-23.1.9-rc-108922: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 23.1.9-rc failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants