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.

Sign up for GitHub

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

Jump to bottom

util/must: merge with logcrash assertions #107428

Closed
erikgrinaker opened this issue Jul 23, 2023 · 2 comments
Closed

util/must: merge with logcrash assertions #107428

erikgrinaker opened this issue Jul 23, 2023 · 2 comments
Assignees
@erikgrinaker
Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team

Comments

@erikgrinaker
Copy link
Contributor

erikgrinaker commented Jul 23, 2023 •
edited
Loading

Currently, the logcrash package has some functionality that overlaps must: ReportOrPanic which panics in non-release builds and otherwise sends a Sentry notification, and the cluster setting debug.panic_on_failed_assertions which makes ReportOrPanic always panic.

ReportOrPanic should be replaced by must assertions. The cluster setting may either be replaced entirely by COCKROACH_FATAL_ASSERTIONS, or must may be changed to respect the cluster setting instead. Using a cluster setting requires propagating cluster settings to must, and won't work for assertion failures that happen before KV is functional and cluster settings are propagated. The environment variable, on the other hand, requires a node restart.

My preference is for the environment variable: we rarely, if ever, expect users to enable fatal assertions, so the inconvenience of a node restart is not a big problem, and environment variables are more reliable and easier to propagate.

Jira issue: CRDB-30038
@erikgrinaker erikgrinaker added C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) A-testing Testing tools and infrastructure T-testeng TestEng Team labels Jul 23, 2023
@erikgrinaker erikgrinaker self-assigned this Jul 23, 2023
@blathers-crl
Copy link

blathers-crl bot commented Jul 23, 2023

cc @cockroachdb/test-eng

This was referenced Jul 23, 2023
Merged
Closed
@erikgrinaker
Copy link
Contributor Author

must is no more, see #108272.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@erikgrinaker erikgrinaker

Labels
A-testing Testing tools and infrastructure C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) T-testeng TestEng Team
Projects
No open projects
Test Engineering
Status: Done
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

*: use must instead of logcrash.ReportOrPanic erikgrinaker/cockroach
1 participant
@erikgrinaker