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

Don't panic on unknown raft ops #17732

Merged
merged 5 commits into from
Nov 30, 2022

Conversation

raskchanky
Copy link
Contributor

No description provided.

@@ -593,19 +593,19 @@ 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 _, log := range logs {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed all of these so it wouldn't conflict with the imported log package.

physical/raft/fsm.go Outdated Show resolved Hide resolved
@raskchanky raskchanky requested a review from ncabatoff November 29, 2022 22:21
@raskchanky raskchanky requested review from ncabatoff and removed request for ncabatoff November 30, 2022 22:04
@@ -672,7 +673,10 @@ func (f *FSM) ApplyBatch(logs []*raft.Log) []interface{} {
go f.restoreCb(context.Background())
}
default:
return fmt.Errorf("%q is not a supported transaction operation", op.OpType)
if _, ok := f.unknownOpTypes.Load(op.OpType); !ok {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok with this, but I'll note that I don't think a sync.Map or any locking is necessary. Even if raft-lib were to invoke ApplyBatch multiple times concurrently (which I don't think would make sense), boltdb Update transactions can't run concurrently with one another.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, of course. I noticed that the fsm itself grabbed a read lock before running the update transaction, so that's what got me thinking that some kind of race protection was necessary.

@raskchanky raskchanky merged commit c9b4300 into main Nov 30, 2022
@raskchanky raskchanky deleted the dont-panic-on-unknown-raft-ops-try-again branch November 30, 2022 23:38
raskchanky added a commit that referenced this pull request Dec 22, 2022
* Don't panic on unknown raft ops

* avoid excessive logging

* track at the struct level, not the function level

* add changelog
raskchanky added a commit that referenced this pull request Dec 22, 2022
* Don't panic on unknown raft ops

* avoid excessive logging

* track at the struct level, not the function level

* add changelog
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
* Don't panic on unknown raft ops

* avoid excessive logging

* track at the struct level, not the function level

* add changelog
miagilepner added a commit that referenced this pull request Mar 18, 2024
miagilepner added a commit that referenced this pull request Mar 19, 2024
* Revert "Don't panic on unknown raft ops (#17732)"

This reverts commit c9b4300.

* add test for panic

* add back changelog

* add godoc for test

* log -> l

* changelog

* Apply suggestions from code review

Co-authored-by: Josh Black <raskchanky@gmail.com>

---------

Co-authored-by: Josh Black <raskchanky@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants