Skip to content

workload: remove initial prefix from bank workload payload#102907

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
renatolabs:rc/drop-initial-prefix-bank-workload
May 9, 2023
Merged

workload: remove initial prefix from bank workload payload#102907
craig[bot] merged 1 commit intocockroachdb:masterfrom
renatolabs:rc/drop-initial-prefix-bank-workload

Conversation

@renatolabs
Copy link
Copy Markdown

@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
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
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 PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only 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
Copy Markdown
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
Copy Markdown
Author

TFTRs!

bors r=herkolategan,srosenberg

@renatolabs
Copy link
Copy Markdown
Author

bors r-

@craig
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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 PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants