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

State cb #545

Merged
merged 2 commits into from
Dec 5, 2023
Merged

State cb #545

merged 2 commits into from
Dec 5, 2023

Conversation

MathieuBordere
Copy link
Contributor

@MathieuBordere MathieuBordere commented Dec 4, 2023

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.

Mathieu Borderé added 2 commits December 4, 2023 13:33
Fixes:
1. Tests were no longer able to test the condition where WAL frames were
   being applied while a tx was open, because the test didn't run long
   enough.
2. A race in the tests where some systems generate a barrier while a
   barrier was active, making it difficult to wait for the right log
   entry.

Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
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.

Signed-off-by: Mathieu Borderé <mathieu.bordere@canonical.com>
Copy link

codecov bot commented Dec 4, 2023

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 ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@cole-miller cole-miller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@MathieuBordere
Copy link
Contributor Author

MathieuBordere commented Dec 5, 2023

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 ...

@MathieuBordere MathieuBordere merged commit 6f62101 into canonical:master Dec 5, 2023
11 of 12 checks passed
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.

Jepsen: assertion failure in vfs.c
2 participants