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

Re-add schemaOnly parameter when applying raft log entries and ensure we reloadDB on catching up #4897

Merged
merged 2 commits into from May 14, 2024

Conversation

reyreaud-l
Copy link
Contributor

@reyreaud-l reyreaud-l commented May 10, 2024

What's being changed:

Reverts #4882. Re-add the schema only check in the db.apply function and add a new atomic dbAndRaftAligned that is set to true once RAFT has caught up and re-applied log entries after a restart and we no longer need to use schema only.

This is necessary because on restart RAFT will re-apply log entries from disk to update the in-memory representation. As our DB operations are not truly idempotent we want to use a schemaOnly parameter to avoid updating the DB and only update the in-memory schema representation.

For example given the following order of operations (Operation -> In memory SchemaState | DBState)

1. AddClass(c1)        -> classes=[c1] | object["c1"] = []
2. DeleteClass(c1)     -> classes=[]   | object[]     = []
3. AddClass(c1)        -> classes=[c1] | object["c1"] = []
4. AddObject(c1, obj1) -> classes=[c1] | object["c1"] = [obj1]

If a node is to restart, it will replay the raft log entries (which are 1, 2 and 3 above) but not the data related operations as these are not handled by RAFT. On restarting a node the following sequence is happening

0. Initial state            -> classes=[]   | object["c1"] = [obj1]
2. (Replay) AddClass(c1)    -> classes=[c1] | object["c1"] = [obj1]
3. (Replay) DeleteClass(c1) -> classes=[]   | object[]     = [] <-- Here we lose data by *also* dropping obj1
4. (Replay) AddClass(c1)    -> classes=[c1] | object[]     = []

With this change when we are replaying log operation we avoid updating the DB until we have caught up and then reload the DB representation to ensure we have everything loading as is represented in the up-to-date schema.

Review checklist

  • Documentation has been updated, if necessary. Link to changed documentation:
  • Chaos pipeline run or not necessary. Link to pipeline:
  • All new code is covered by tests where it is reasonable.
  • Performance tests have been run or not necessary.

@reyreaud-l reyreaud-l changed the title Revert "raft: remove schemaOnly and rename lastAppliedIndexOnStart" Re-add schemaOnly parameter when applying raft log entries and ensure we reloadDB on catching up May 10, 2024
cluster/store/store.go Outdated Show resolved Hide resolved
cluster/store/store.go Outdated Show resolved Hide resolved
moogacs
moogacs previously approved these changes May 13, 2024
cluster/store/store.go Outdated Show resolved Hide resolved
adapters/repos/db/nodes.go Outdated Show resolved Hide resolved
@reyreaud-l reyreaud-l force-pushed the revert-4882-refactor-db-open-4 branch 3 times, most recently from 7ee363d to 660ca9c Compare May 13, 2024 12:50
@reyreaud-l reyreaud-l requested a review from moogacs May 14, 2024 05:48
@reyreaud-l reyreaud-l force-pushed the revert-4882-refactor-db-open-4 branch from 660ca9c to a12d014 Compare May 14, 2024 06:05
Copy link

sonarcloud bot commented May 14, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@reyreaud-l reyreaud-l merged commit 8dda171 into stable/v1.25 May 14, 2024
42 checks passed
@reyreaud-l reyreaud-l deleted the revert-4882-refactor-db-open-4 branch May 14, 2024 07:43
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.

None yet

3 participants