-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add an optional ConfigurationStore interface #339
Conversation
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.
Looks great @briankassouf!
A couple minor Qs/suggestions inline.
Co-Authored-By: Paul Banks <[email protected]>
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.
Nice! LGTM!
// in memory and snapshotting periodically. By storing configuration changes, the | ||
// persistent FSM state can behave as a complete snapshot, and be able to recover | ||
// without an external snapshot just for persisting the raft configuration. | ||
type ConfigurationStore interface { |
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.
Random drive-by comment: if this interface is only useful for FSMs, should you embed the FSM interface in here as well?
I tend to find that to be a fairly self-documenting behavior I appreciate.
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.
Good idea, i'll add that
FSMs that satisfy this interface will be sent configuration change updates after they are fully committed to the log