-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
txnwait: do not require vmodule to log deadlocks in a workload #99315
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @arulajmani)
pkg/kv/kvserver/txnwait/queue.go
line 722 at r1 (raw file):
p1, p2 := pusheePriority, pusherPriority if p1 < p2 || (p1 == p2 && bytes.Compare(req.PusheeTxn.ID.GetBytes(), req.PusherTxn.ID.GetBytes()) < 0) { log.Infof(
Do you think we should wrap this in an EveryN? Maybe only log it once a second, and otherwise retain the log.VEventf
? I think you can continue to use log.VEventf
and then use level=0
for the analogous call to log.Infof
, but please double-check if you decide to write this like that.
This still seems like a good idea. I imagine we'll thank ourselves for adding in this logging at some point in the future. |
Deadlocks in a workload are rare (and bad) enough that they should be logged at the level of Info, instead of under vmodule. Epic: none Release note: None
4b2b4b8
to
b5681fa
Compare
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.
Better late than never I guess 😅
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
pkg/kv/kvserver/txnwait/queue.go
line 722 at r1 (raw file):
I think you can continue to use
log.VEventf
and then uselevel=0
for the analogous call tolog.Infof
, but please double-check if you decide to write this like that.
You're correct; verified that this is indeed how it works.
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.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @arulajmani)
Thanks! bors r=nvanbenschoten |
Deadlocks in a workload are rare (and bad) enough that they should be logged at the level of Info, instead of under vmodule.
Epic: none
Release note: None