Repository navigation
Conversation
|
Thanks. I checked it and found no correctness problems; all tests pass. But I'm not sure we need it:
Since this changes how in-flight state is cleared on NewView, I'd rather not take that risk for this gain. Have you seen growth since #739 with default WAL sizes? If so, could you add it to #736? Otherwise I'd suggest closing this and #735, and closing #736 as fixed by #739. (Written by Claude Code.) |
You're probably right. I'll take a break to think. |
The WAL had a truncation point only on a pre-prepare. A live node that does not receive pre-prepares still writes ViewChange, NewView and sync NewView records, so its truncateIndex never moved and its WAL grew at every file rotation. Treat a ViewChange, NewView or sync NewView record as a truncation point when no proposal is in flight. Use the same definition of in-flight as the view data message: a proposal at or before the last decision is not in flight, even when InFlightData still holds it until the next pre-prepare or sync. Keep the prepared proposal under condition B, and never truncate on a commit. Do not clear InFlightData on NewView: the checkpoint decides the flag, so the in-flight state does not have to be mutated to make the record a truncation point. Add an integration test with a node that loses only pre-prepares, stays connected, and goes through view changes. The test fails on main and passes with this change. Add another test for a node that decided a block and then lost pre-prepares. Without the decided-proposal check the WAL grew from 3 to 19 files; with it it stays at 2. Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
db6e372 to
1181e1b
Compare
I found a case the current change still misses. A node decides a block, then loses pre-prepares. InFlightData still holds the decided proposal. hasInFlightProposal() looked at the raw pointer and returned true, so every ViewChange and NewView record kept the WAL. But getInFlight() already treats a proposal at the last decision as not in flight for the view data message. The new integration test decides one block, drops pre-prepares, and forces 8 view changes. On the current PR the WAL grows from 3 to 19 files with a 32-byte file size. With the fix it stays at 2. The fix uses the same getInFlight check for the truncation flag. I also removed InFlight.clear() on NewView: with the checkpoint deciding the flag, there is no need to mutate the in-flight state. That addresses the risk you raised. I agree a stable leader with no pre-prepares writes no view records; this test is about the truncation point after a view change, not about stable-leader growth. Could you take another look? |
The WAL had a truncation point only on a pre-prepare. A live node that does not
receive pre-prepares still writes ViewChange, NewView and sync NewView records,
so its truncateIndex never moved and its WAL grew at every file rotation.
Treat a ViewChange, NewView or sync NewView record as a truncation point when no
proposal is in flight. Use the same definition of in-flight as the view data
message: a proposal at or before the last decision is not in flight, even when
InFlightData still holds it until the next pre-prepare or sync. Keep the prepared
proposal under condition B, and never truncate on a commit. Do not clear
InFlightData on NewView: the checkpoint decides the flag, so the in-flight state
does not have to be mutated to make the record a truncation point.
Add an integration test with a node that loses only pre-prepares, stays
connected, and goes through view changes. The test fails on main and passes with
this change.
Add another test for a node that decided a block and then lost pre-prepares.
Without the decided-proposal check the WAL grew from 3 to 19 files; with it it
stays at 2.
#736