canonical / dqlite

Embeddable, replicated and fault-tolerant SQL engine.
https://dqlite.io
Other
3.77k stars 213 forks source link

State cb #545

Closed MathieuBordere closed 7 months ago

MathieuBordere commented 7 months ago

The state_cb is triggered immediately when the raft state changes, instead of the monitor_cb, that is only polling for the state change at the start of a loop iteration. This avoids some bugs where dqlite has to perform cleanups on leadership loss, but wasn't informed yet of the leadership loss.

fixes #541

Note: When we release a new dqlite version, we will also need an accompanying new raft version that supports registering the state_cb.

codecov[bot] commented 7 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (956de45) 61.37% compared to head (9b3ac55) 61.29%.

Files Patch % Lines
src/server.c 66.66% 0 Missing and 3 partials :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #545 +/- ## ========================================== - Coverage 61.37% 61.29% -0.08% ========================================== Files 34 34 Lines 6842 6834 -8 Branches 2031 2031 ========================================== - Hits 4199 4189 -10 Misses 1349 1349 - Partials 1294 1296 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

MathieuBordere commented 7 months ago

Looks good to me, could you elaborate on why the test changes are needed?

The unchanged tests only waited for the leader barrier to be applied, so they didn't test an ex-leader trying to apply frames to it's WAL. The changes make the test explicitly wait for the barrier first and then wait for the COMMAND_FRAMES to be applied. In short, the tests didn't hit the shm->shared[i] == 0 assertion anymore when I removed closing of the leader connections in the monitor_cb and now state_cb, and now they do.

This test wasn't adapted after the raft "barrier on become leader" change was introduced because they still passed. Probably there are more cases like this ...