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

kvserver: add SmallEngineBlocks testing knob and metamorphic params #89026

Merged
merged 5 commits into from
Sep 30, 2022

Conversation

erikgrinaker
Copy link
Contributor

@erikgrinaker erikgrinaker commented Sep 29, 2022

@cockroachdb/repl-prs to do the main review, tagging other teams for visibility/review of metamorphic test params.

Resolves #86648.


kvserver: add SmallEngineBlocks testing knob

This patch adds a store testing knob SmallEngineBlocks which
configures Pebble with a block size of 1 byte. This will store every key
in a separate block, which can provoke bugs in time-bound iterators.

Release note: None

sql/logictest: add metamorphic test param for small engine blocks

Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None

kvserver/rangefeed: add metamorphic test param for small engine blocks

Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None

kvserver/gc: add metamorphic test param for small engine blocks

Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None

backupccl: add metamorphic test param for small engine blocks

Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Member

@tbg tbg left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r1, 1 of 1 files at r2, 2 of 2 files at r3, 2 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @msbutler, and @yuzefovich)


-- commits line 21 at r3:
Any reason they're not metamorphic? I assume a block size of one can also hide bugs, though I'll admit that that's somewhat less likely.


pkg/server/config.go line 705 at r1 (raw file):

				cfg.SoftSlotGranter,
			}
			if storeKnobs.SmallEngineBlocks {

I'm surprised the in-mem path and this one are so different in how options are configured.
Ah, seeing below that BlockSize() is something you added that does the same thing.

This patch adds a store testing knob `SmallEngineBlocks` which
configures Pebble with a block size of 1 byte. This will store every key
in a separate block, which can provoke bugs in time-bound iterators.

Release note: None
Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None
Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None
Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @msbutler, @tbg, and @yuzefovich)


-- commits line 21 at r3:

Previously, tbg (Tobias Grieger) wrote…

Any reason they're not metamorphic? I assume a block size of one can also hide bugs, though I'll admit that that's somewhat less likely.

May as well, done.


pkg/server/config.go line 705 at r1 (raw file):

Previously, tbg (Tobias Grieger) wrote…

I'm surprised the in-mem path and this one are so different in how options are configured.
Ah, seeing below that BlockSize() is something you added that does the same thing.

Yeah, I suspect the main engine initialization code path got pulled out of storage.Open to have direct access to the Pebble config. Could probably be cleaned up, but not doing that now.

Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None
Uses a Pebble block size of 1 byte, to provoke bugs in time-bound
iterators.

Release note: None
Copy link
Member

@yuzefovich yuzefovich 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 r7.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, @msbutler, and @tbg)


pkg/sql/logictest/logic.go line 556 at r7 (raw file):

	// smallEngineBlocks configures Pebble with a block size of 1 byte, to provoke
	// bugs in time-bound iterators.
	smallEngineBlocks = util.ConstantWithMetamorphicTestBool("logictest-small-engine-blocks", false)

Will this make tests run noticeably slower? Specifically I'm thinking about SQLLite test corpus which usually inserts on the order of few thousands of rows into the table.

Copy link
Contributor Author

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @msbutler, @tbg, and @yuzefovich)


pkg/sql/logictest/logic.go line 556 at r7 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

Will this make tests run noticeably slower? Specifically I'm thinking about SQLLite test corpus which usually inserts on the order of few thousands of rows into the table.

Yes, it will make tests somewhat slower. For the fakedist suite, I believe it was slowed down by something like 20-30%.

Copy link
Collaborator

@msbutler msbutler left a comment

Choose a reason for hiding this comment

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

bulk changes look fine!

Copy link
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

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

Logic test change LGTM.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @aliher1911, @erikgrinaker, and @tbg)


pkg/sql/logictest/logic.go line 556 at r7 (raw file):

Previously, erikgrinaker (Erik Grinaker) wrote…

Yes, it will make tests somewhat slower. For the fakedist suite, I believe it was slowed down by something like 20-30%.

I see. We'll probably need to increase the timeout for that test corpus (currently it's not a problem since we don't set crdb_test build flag for nightlies there, but I do want to bring it back).

@erikgrinaker
Copy link
Contributor Author

All CI failures are known flakes. TFTRs!

bors r+

@craig
Copy link
Contributor

craig bot commented Sep 30, 2022

Build succeeded:

@craig craig bot merged commit 3e03176 into cockroachdb:master Sep 30, 2022
@blathers-crl
Copy link

blathers-crl bot commented Sep 30, 2022

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 e0e5911 to blathers/backport-release-22.2-89026: 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 22.2.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

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.

storage: metamorphic tests with small Pebble blocks
5 participants