Skip to content
This repository was archived by the owner on Mar 4, 2024. It is now read-only.

replication: Tweak applyLeaderCb cleanup #352

Merged
merged 1 commit into from
Jan 5, 2023

Conversation

cole-miller
Copy link
Contributor

Closes #338. After thinking about it for a while, I've convinced myself that appendLeaderCb can just skip truncating the log if the appended entries (for which the disk write was cancelled) are not present, because this can occur in a totally benign way. For example, we could start an append request for an entry at index 16 and then start another for index 17 before the first request is finished. Then we could cancel those two requests in the same order they were submitted, and the appendLeaderCb for the second request won't see any entry at index 17 to be truncated. And in fact this seems to be just what happened to cause that Jepsen failure.

(It would be elegant to cancel queued requests in LIFO order, so that each callback sees the log in the same state it was just after it appended its own entries. That would obviate the need for the logLastIndex check here. But I'm a little nervous about unanticipated consequences from changing that ordering.)

Signed-off-by: Cole Miller cole.miller@canonical.com

Signed-off-by: Cole Miller <cole.miller@canonical.com>
@codecov
Copy link

codecov bot commented Jan 4, 2023

Codecov Report

Merging #352 (8b9c95c) into master (3c69054) will decrease coverage by 0.01%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##           master     #352      +/-   ##
==========================================
- Coverage   84.08%   84.06%   -0.02%     
==========================================
  Files          50       50              
  Lines        9454     9456       +2     
  Branches     2521     2523       +2     
==========================================
  Hits         7949     7949              
  Misses        934      934              
- Partials      571      573       +2     
Impacted Files Coverage Δ
src/replication.c 77.61% <75.00%> (-0.07%) ⬇️
src/fixture.c 94.62% <0.00%> (-0.10%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@MathieuBordere MathieuBordere self-requested a review January 5, 2023 07:44
@MathieuBordere
Copy link
Contributor

nice catch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion index <= logLastIndex(l) failed
2 participants