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

kvserverpb: mark probe requests' replicated cmds as IsTrivial #102953

Merged
merged 1 commit into from
Jun 9, 2023

Conversation

tbg
Copy link
Member

@tbg tbg commented May 9, 2023

That way, they won't each request their own replicaAppBatch. They were wildly
inefficient to apply before this commit, which played a (minor) role in a recent
incident1.

To validate this change, I ran an experiment that set up a range with 10 million unapplied probes. This PR more than halved how long it took to apply the entries.

Without this change:

image

With this change:

image

Touches https://github.com/cockroachlabs/support/issues/2287.
Closes #103905.

Epic: none
Release note: None

Footnotes

  1. https://github.com/cockroachlabs/support/issues/2287

@blathers-crl
Copy link

blathers-crl bot commented May 9, 2023

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

@cockroach-teamcity
Copy link
Member

This change is Reviewable

That way, they won't each request their own `replicaAppBatch`. They were wildly
inefficient to apply before this commit, which played a role in a recent
incident[^1].

[^1]: cockroachlabs/support#2287

Epic: none
Release note: None
@tbg tbg force-pushed the probes-are-trivial branch 2 times, most recently from 6ef1fb4 to f207e49 Compare June 6, 2023 09:52
@tbg tbg requested a review from pav-kv June 6, 2023 09:52
@tbg tbg marked this pull request as ready for review June 6, 2023 09:56
@tbg tbg requested a review from a team as a code owner June 6, 2023 09:56
tbg added a commit to tbg/cockroach that referenced this pull request Jun 6, 2023
This is the test used for cockroachdb#102953.

Epic: none
Release note: none
@tbg tbg added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Jun 6, 2023
Copy link
Collaborator

@pav-kv pav-kv left a comment

Choose a reason for hiding this comment

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

The first sentence of the commit message is broken, please fix.

@tbg
Copy link
Member Author

tbg commented Jun 9, 2023

bors r=pavelkalinnikov

@craig
Copy link
Contributor

craig bot commented Jun 9, 2023

Build succeeded:

@craig craig bot merged commit 9080fc9 into cockroachdb:master Jun 9, 2023
4 checks passed
@blathers-crl
Copy link

blathers-crl bot commented Jun 9, 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 f207e49 to blathers/backport-release-22.2-102953: 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 dev-inf.

tbg added a commit to tbg/cockroach that referenced this pull request Jun 12, 2023
This is the test used for cockroachdb#102953.

Epic: none
Release note: none
tbg added a commit to tbg/cockroach that referenced this pull request Jun 20, 2023
This is the test used for cockroachdb#102953.

Epic: none
Release note: none
craig bot pushed a commit that referenced this pull request Jun 20, 2023
104401: kvserver: add TestCreateManyUnappliedProbes r=pavelkalinnikov a=tbg

This is the test used for #102953.

This PR also makes modest progress on #75729, not by making log application
stand-alone, but by making it somewhat less convoluted to manufacture log
entries programmatically. It would be desirable to be able to test the
replication layer in CRDB in the same way that Raft allows via the
InteractionEnv[^1].

[^1]: https://github.com/etcd-io/raft/blob/6bf4f7fe3122b064e0a0d76314298dca6f379fc7/interaction_test.go

Epic: none
Release note: none


105083: kvcoord: Release catchup reservation before re-acquire attempt r=miretskiy a=miretskiy

Release catchup scan reservation prior to attemt to re-acquire it.  Failure to do so could result in a stuck mux rangefeed when enough ranges encounter an error, such as range split, prior to receiving the first checkpoint event, that would cause additional attempts to acquire catchup scan quota.

Fixes #105058

Release note (bug fix): Fix a bug in mux rangefeed implementation that may cause mux rangefeed to become stuck if enough ranges encounter certain error concurrently.

Co-authored-by: Tobias Grieger <tobias.b.grieger@gmail.com>
Co-authored-by: Yevgeniy Miretskiy <yevgeniy@cockroachlabs.com>
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.

kvserver: make probe request application cheap
3 participants