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.

Sign up for GitHub

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

Jump to bottom

RaftActor shouldn't delete committed or non-conflict entries #151

Merged
merged 18 commits into from
Apr 27, 2022
Merged

RaftActor shouldn't delete committed or non-conflict entries #151

merged 18 commits into from
Apr 27, 2022

Conversation

xirc
Copy link
Contributor

@xirc xirc commented Apr 25, 2022 •
edited
Loading

Closes #152

There might be two places to be able to fix this bug:

  • AppendedEntries replay or
  • AppendEntries handling

This PR changes AppendEntries handling to fix this bug.
pros:

  • possible to maintain backward compatibility (changing event replay might break the existing Raft log)
  • reduce required event storage space since RaftActor persists only new entries (including conflicts)

TODO

  • Fix main code
  • Write unit tests
    • RaftMemberData.resolveNewLogEntries
    • ReplicatedLog.findConflict
    • ReplicatedLog.truncateAndAppend
    • receiving AppendEntries
    • RaftActor can read old AppenddedEntries event
  • Run one AZ failure gatling scenario (not included in this project) for verifying this bug fixed
  • Write an issue describing this bug
  • Write CHANGELOG

@xirc xirc requested a review from negokaz April 25, 2022 10:47
@xirc
Copy link
Contributor Author

xirc commented Apr 25, 2022

@negokaz
As our team discussed, we have to fix this bug. Our team created a draft (not the exact same as this PR's code) fixing this bug and also verified this strategy might be the right direction. I've created a fix patch for this bug (is possible to merge but not unit-tested yet). Could you do a quick review of the code? I'm going to run the Gatling scenario and also write tests. I've already verified that existing tests pass.

@xirc xirc force-pushed the fix-append-entries-handling branch from 38eaa66 to f3c9552 Compare April 25, 2022 11:12
negokaz
negokaz reviewed Apr 26, 2022
View reviewed changes
Copy link
Contributor

@negokaz negokaz left a comment

Choose a reason for hiding this comment

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

@xirc
Great!
I have reviewed the changes in main and left some comments.

src/main/scala/lerna/akka/entityreplication/raft/model/ReplicatedLog.scala Outdated Show resolved Hide resolved
src/main/scala/lerna/akka/entityreplication/raft/RaftMemberData.scala Outdated Show resolved Hide resolved
src/main/protobuf/cluster_replication.proto Show resolved Hide resolved
@xirc xirc force-pushed the fix-append-entries-handling branch from fb4ce14 to f8e952b Compare April 26, 2022 09:06
@xirc xirc force-pushed the fix-append-entries-handling branch from 040c577 to b744b98 Compare April 27, 2022 01:08
@xirc xirc marked this pull request as ready for review April 27, 2022 04:59
negokaz
negokaz approved these changes Apr 27, 2022
View reviewed changes
Copy link
Contributor

@negokaz negokaz left a comment

Choose a reason for hiding this comment

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

Awesome! LGTM 👍

@negokaz negokaz merged commit 0acd526 into master Apr 27, 2022
@negokaz negokaz deleted the fix-append-entries-handling branch April 27, 2022 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@negokaz negokaz negokaz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

RaftActor might delete committed entries
2 participants
@xirc @negokaz