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

WIP: Durability issue fix in RAFT layer. #14406

Closed
wants to merge 2 commits into from

Conversation

ptabor
Copy link
Contributor

@ptabor ptabor commented Aug 30, 2022

... still working on tests

r.logger.Infof("Updating progress: %v", e.Index)
r.prs.Progress[r.id].MaybeUpdate(e.Index)
}
r.maybeCommit() // TODO: Consider moving to stepLeader loop...
Copy link
Member

Choose a reason for hiding this comment

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

This isn't thread(goroutine) safe, because the raft may do it concurrently.

Copy link
Member

@ahrtr ahrtr Aug 31, 2022

Choose a reason for hiding this comment

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

No need to call r.maybeCommit() if you follow comment

@@ -573,6 +573,11 @@ func (r *raft) advance(rd Ready) {
if len(rd.Entries) > 0 {
e := rd.Entries[len(rd.Entries)-1]
r.raftLog.stableTo(e.Index, e.Term)
if r.prs.Progress[r.id] != nil {
r.logger.Infof("Updating progress: %v", e.Index)
r.prs.Progress[r.id].MaybeUpdate(e.Index)
Copy link
Member

Choose a reason for hiding this comment

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

This isn't thread(goroutine) safe either, because it might trigger the map data-race error.

Copy link
Member

Choose a reason for hiding this comment

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

You might need to construct a pb.MsgAppResp message and call (*node)Step()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Was looking for a way to do this within 'raft' - without exiting to the 'node' level,
but calling r.Step() would be risky.

@ahrtr
Copy link
Member

ahrtr commented Aug 31, 2022

FYI. ahrtr@7ab0842

@ptabor
Copy link
Contributor Author

ptabor commented Aug 31, 2022

Agree that ahrtr@7ab0842 variant is better (i.e. explicit advancement of the state using .Step(Message) method. Let's iterate there.

@ahrtr
Copy link
Member

ahrtr commented Aug 31, 2022

Agree that ahrtr@7ab0842 variant is better (i.e. explicit advancement of the state using .Step(Message) method. Let's iterate there.

Sure, thx

@ptabor
Copy link
Contributor Author

ptabor commented Sep 2, 2022

Closing as #14411 is superior to this.

@serathius serathius closed this Sep 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants