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

Add op type to panics #4108

Merged
merged 1 commit into from Apr 27, 2023
Merged

Add op type to panics #4108

merged 1 commit into from Apr 27, 2023

Conversation

neilalexander
Copy link
Member

This updates the panic messages on applying meta entries to include the faulty op type, so that we can better work out what's going on in these cases.

Signed-off-by: Neil Twigg neil@nats.io

@neilalexander neilalexander requested a review from a team as a code owner April 27, 2023 09:58
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

I would reformat the error as suggested.

@@ -1728,7 +1728,7 @@ func (js *jetStream) applyMetaEntries(entries []*Entry, ru *recoveryUpdates) (bo
js.processUpdateStreamAssignment(sa)
}
default:
panic("JetStream Cluster Unknown meta entry op type")
panic(fmt.Sprintf("JetStream Cluster Unknown meta entry op type! %v", entryOp(buf[0])))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
panic(fmt.Sprintf("JetStream Cluster Unknown meta entry op type! %v", entryOp(buf[0])))
panic(fmt.Sprintf("JetStream Cluster Unknown meta entry op type: %v!", entryOp(buf[0])))

Copy link
Member

Choose a reason for hiding this comment

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

Should we log the more detailed stuff then panic with a simple unknown opcode?

This will not be a bug, fyi, this will be a fault, data corruption, etc. Hence the panic.

@@ -2687,7 +2687,7 @@ func (js *jetStream) applyStreamEntries(mset *stream, ce *CommittedEntry, isReco
}
}
default:
panic("JetStream Cluster Unknown group entry op type!")
panic(fmt.Sprintf("JetStream Cluster Unknown group entry op type! %v", op))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
panic(fmt.Sprintf("JetStream Cluster Unknown group entry op type! %v", op))
panic(fmt.Sprintf("JetStream Cluster Unknown group entry op type: %v!", op))

Signed-off-by: Neil Twigg <neil@nats.io>
Copy link
Member

@Jarema Jarema left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit 3feb9f7 into main Apr 27, 2023
2 checks passed
@neilalexander neilalexander deleted the neil/oppanic branch April 27, 2023 12:02
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

3 participants