-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
VAULT-23553: Revert "Don't panic on unknown raft ops" #25991
Conversation
CI Results: |
physical/raft/fsm.go
Outdated
@@ -678,24 +677,24 @@ func (f *FSM) ApplyBatch(logs []*raft.Log) []interface{} { | |||
// Do the unmarshalling first so we don't hold locks | |||
var latestConfiguration *ConfigurationValue | |||
commands := make([]interface{}, 0, numLogs) | |||
for _, l := range logs { |
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.
Nit: I have a very mild preference that variable names not shadow package names, just for the sake of readability, which is why I changed all these log
references in the original PR. Not a big deal by any means.
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.
Updated to l
Build Results: |
Co-authored-by: Josh Black <[email protected]>
r := recover() | ||
require.NotNil(t, r) | ||
require.Contains(t, r, "failed to store data") | ||
}() |
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.
This is a very cool trick! I didn't know you could do that.
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.
This PR reverts the change as part of undo logs, and adds a test to verify that an unknown operation will panic.