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: don't repropose probes #102956

Closed
wants to merge 1 commit into from
Closed

Conversation

tbg
Copy link
Member

@tbg tbg commented May 9, 2023

We saw this likely contribute to OOMs during log application in
https://github.com/cockroachlabs/support/issues/2287.

Touches #98563.

Epic: CRDB-25503
Release note: None

We saw this likely contribute to OOMs during log application in
cockroachlabs/support#2287.

Touches cockroachdb#98563.

Epic: CRDB-25503
Release note: None
@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

@tbg
Copy link
Member Author

tbg commented May 10, 2023

With the change as is, probes get an ambiguous result after 3s, which means a new probe can start after 3s instead of after 60s, so while avoiding quadratic build-up in the log, we get 20x more in-mem proposals and the log will grow at 20x the rate. Not sure this is a great trade-off.

We should probably repropose a probe exactly once, and give it an ambiguous result after 60s.

We should also commonly be able to avoid reproposing. If the local raft instance is the leader (as it mostly is), if it has unapplied log entries, we might as well wait for those to get applied first before reproposing. Also, commands that were proposed to a local leader and not dropped (i.e. if we handled ErrProposalDropped) are known to be in the log and could only be replaced if local leadership were lost, for which we get an event.

Long story short, what we're doing now is extremely inefficient. Log build-up during outages is totally avoidable.

In the same vein, we keep commands in the command map (r.mu.proposals) even after the callers have given up. It is true that we need to keep the latches in place until we have proven that the command can no longer apply. But here too we could be a lot more effective if, instead of reproposing the exact command multiple times (and doing so for possibly many in-flight commands), we'd instead send a single probe only. This would cause more spurious failures in the case in which the commands actually never made it into the log: we need to be much better with our tracking of such things to avoid flakes during the early life of ranges (while elections are happening, etc).

But this all can be done, if we are in the mood for a larger rework.

@tbg
Copy link
Member Author

tbg commented Jul 3, 2023

#105896 goes the other way and removes the timeout on probes to avoid the quadratic build-up. We can still discuss optimizing our reproposal behavior, but it's independent of probes at this point.

@tbg tbg closed this Jul 3, 2023
@tbg tbg deleted the no-repropose-probe branch July 3, 2023 13:22
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.

2 participants