-
Notifications
You must be signed in to change notification settings - Fork 712
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
Stale Read checks include comparing node's Applied Index with Leader Commit Index #1673
base: master
Are you sure you want to change the base?
Conversation
store/store.go
Outdated
@@ -1806,7 +1807,8 @@ func (s *Store) isStaleRead(freshness int64) bool { | |||
if freshness == 0 || s.raft.State() == raft.Leader { | |||
return false | |||
} | |||
return time.Since(s.raft.LastContact()).Nanoseconds() > freshness | |||
return time.Since(s.raft.LastContact()).Nanoseconds() > freshness && | |||
s.raft.CommitIndex() == s.DBAppliedIndex() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having s.raft.CommitIndex() == s.DBAppliedIndex()
makes the condition too strict, especially for large updates that can still finish within the allowed freshness.
Could we instead add aTime
field in Store
that gets updated to LastContact
when there is no new index, and when there is, to whenever the index gets applied?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed s.DBAppliedIndex()
is protected by a mutex. Would it make sense to replace it with a https://pkg.go.dev/sync/atomic#Uint64?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're the second person to suggest this. @jtackaberry did too.
Makes sense, but I'll do that in a separate PR.
No description provided.