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

workload: remove initial prefix from bank workload payload #102907

Merged

Conversation

renatolabs
Copy link
Collaborator

@renatolabs renatolabs commented May 8, 2023

An initial- prefix is added to the payload column of the bank table when the workload is initialized. It was introduced about 6 years ago [1] and its purpose at the time is not clear. There are two main problems with it:

  • the initial- prefix suggests the payload may be updated, but that actually never happens.
  • as currently implemented, it assumes that the payload-bytes command line flag is at least len([]byte("initial-")). Passing a lower value to that command line flag leads to a panic. This is an implicit assumption that should not exist.

This changes the row generation function so that payload-bytes bytes are randomly generated and inserted into the payload column, without the initial- prefix.

[1] d49d535

Epic: none

Release note: None

@renatolabs renatolabs requested a review from a team as a code owner May 8, 2023 20:21
@renatolabs renatolabs requested review from herkolategan and srosenberg and removed request for a team May 8, 2023 20:21
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@herkolategan herkolategan left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @srosenberg)

@renatolabs renatolabs added the backport-23.1.x Flags PRs that need to be backported to 23.1 label May 9, 2023
@renatolabs renatolabs force-pushed the rc/drop-initial-prefix-bank-workload branch from 7ef0474 to ad8bd70 Compare May 9, 2023 13:36
Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

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

:lgtm:

It was introduced about 6 years ago [1] and its purpose at the time is not clear.

Looking at that PR, I am also not sure. Maybe it was intended to be used for benchmarking? E.g., csv workload has a microbenchmarks that relies on InitialRows.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @herkolategan)

@renatolabs
Copy link
Collaborator Author

TFTRs!

bors r=herkolategan,srosenberg

@renatolabs
Copy link
Collaborator Author

bors r-

@craig
Copy link
Contributor

craig bot commented May 9, 2023

Canceled.

An `initial-` prefix is added to the payload column of the `bank`
table when the workload is initialized. It was introduced about 6
years ago [1] and its purpose at the time is not clear. There are two
main problems with it:

* the `initial-` prefix suggests the payload may be updated, but that
actually never happens.
* as currently implemented, it assumes that the `payload-bytes`
command line flag is at least `len([]byte("initial-"))`. Passing a
lower value to that command line flag leads to a panic. This is an
implicit assumption that should not exist.

This changes the row generation function so that `payload-bytes` bytes
are randomly generated and inserted into the `payload` column, without
the `initial-` prefix.

[1] d49d535

Epic: none

Release note: None
@renatolabs renatolabs force-pushed the rc/drop-initial-prefix-bank-workload branch from ad8bd70 to 90a9904 Compare May 9, 2023 17:18
@renatolabs
Copy link
Collaborator Author

Looking at that PR, I am also not sure.

I realized I quoted the wrong commit, there's a parent commit that actually introduced the initial- prefix (commit msg and PR descriptions updated). That said, still no mention of the purpose behind the prefix, so nothing really changed.

bors r=herkolategan,srosenberg

@craig
Copy link
Contributor

craig bot commented May 9, 2023

Build succeeded:

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