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

log: clean-up log conflict search #155

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pav-kv
Copy link
Contributor

@pav-kv pav-kv commented Feb 5, 2024

This PR cleans up raftLog.maybeAppend and raftLog.findConflict functions, now that they accept the new logSlice type as input. This replaces a bunch of ad-hoc arithmetics on the raw entries slice with a more understandable "fast-forward" logic on a logSlice.

This also removes a redundant panic in maybeAppend. An equivalent panic is present in the raftLog.append method which is triggered under the same conditions. This panic behaviour is tested in TestLogMaybeAppend, and hasn't changed.

Related to #144

@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 5, 2024

@ahrtr This PR is based on top of testing clean-ups in #151 and #147. Only the last 3 commits need to be reviewed as part of this PR. Feel free to also expedite finishing the blocking PRs. Thank you.

@pav-kv pav-kv requested a review from ahrtr February 5, 2024 22:47
@pav-kv pav-kv force-pushed the cleanup-log-conflict-search branch from 5457d01 to 54925de Compare February 5, 2024 23:00
Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
Use the new entryID type. Move the preceding entry check into the
conflict search method rather than do it outside.

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
@pav-kv pav-kv force-pushed the cleanup-log-conflict-search branch from 54925de to b636292 Compare February 7, 2024 10:09
The append function call right after this does the same check, we don't
need to do this in two places. TestLogMaybeAppend exercises these
panics, and confirms the behaviour is the same after the first panic is
removed.

Signed-off-by: Pavel Kalinnikov <pavel@cockroachlabs.com>
@pav-kv pav-kv force-pushed the cleanup-log-conflict-search branch from b636292 to 85f43e6 Compare February 7, 2024 10:19
@pav-kv
Copy link
Contributor Author

pav-kv commented Feb 8, 2024

CC @nvanbenschoten

Specifically, consider the TODOs I've added in findConflict method. I wonder if this would give any significant boost to log append performance. Currently, we (re-)scan the log upon every append request. I suppose though the win wouldn't be large, because the common/happy path is that the append is right at the tip of the log. But in the unhappy path we can save quite a bit of scanning (and the unhappy path is when the cluster is struggling, so savings can help it recover quicker / not add additional resource pressure).

@erikgrinaker erikgrinaker self-requested a review February 21, 2024 14:24
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.

None yet

1 participant