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

Don't convert to candidate while entries are being persisted, take 2 #467

Merged
merged 2 commits into from
Aug 16, 2023

Conversation

cole-miller
Copy link
Contributor

Fixes #386, reviving the approach from #464 as the first part of the approach worked out in the discussion of #465.

I'm not entirely happy about adding the append_in_flight_count field to struct raft -- it would be more principled to query our raft_io for this information. On the other hand, adding a method for that is a more invasive change that breaks compatibility with older raft_io implementations. So I'm undecided on the best design here.

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

@cole-miller
Copy link
Contributor Author

please test downstream

@codecov
Copy link

codecov bot commented Aug 15, 2023

Codecov Report

Merging #467 (4d482c8) into master (0ea24be) will decrease coverage by 0.02%.
The diff coverage is 76.19%.

❗ Current head 4d482c8 differs from pull request most recent head f65b6e7. Consider uploading reports for the commit f65b6e7 to get more accurate results

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
- Coverage   76.72%   76.71%   -0.02%     
==========================================
  Files          51       51              
  Lines        9686     9696      +10     
  Branches     2476     2476              
==========================================
+ Hits         7432     7438       +6     
- Misses       1088     1094       +6     
+ Partials     1166     1164       -2     
Files Changed Coverage Δ
src/recv_timeout_now.c 45.71% <50.00%> (+0.25%) ⬆️
src/replication.c 69.28% <62.50%> (+0.45%) ⬆️
src/tick.c 85.43% <75.00%> (-0.43%) ⬇️
src/convert.c 84.67% <100.00%> (+0.11%) ⬆️
src/fixture.c 92.76% <100.00%> (+0.02%) ⬆️

... and 2 files with indirect coverage changes

@MathieuBordere
Copy link
Contributor

MathieuBordere commented Aug 16, 2023

Looks good. If I'm being nitty I'd say that a separate commit to initialize append_in_flight_count is a bit over the top.

@MathieuBordere
Copy link
Contributor

Just letting up a balloon. Does the same reasoning hold for a leader, i.e. should we not step down from Leadership when an append request is in flight?

Signed-off-by: Cole Miller <cole.miller@canonical.com>
Signed-off-by: Cole Miller <cole.miller@canonical.com>
@MathieuBordere MathieuBordere merged commit 3494fb4 into canonical:master Aug 16, 2023
18 of 19 checks passed
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.

recvAppendEntries: Assertion r->state == RAFT_FOLLOWER || r->state == RAFT_CANDIDATE failed
2 participants