kv: increment txn.restarts.writetooold on WriteTooOld error #119411
Conversation
WriteTooOld errors were previously counted in either the `txn.restarts.writetooold` metric or the `txn.restarts.writetoooldmulti` metric, depending on when in the lifecycle of a transaction they were thrown. Now that WriteTooOld errors are rarely (never?) deferred, we would like to consolidate these metrics to avoid confusion. As a first step, we now increment `txn.restarts.writetooold` in both cases. In the next release, we can remove the `txn.restarts.writetoooldmulti` metric and its presence in the "Transaction Restarts" graph in the SQL Dashboard. Release note (ui change): The "Write Too Old" metric in the "Transaction Restarts" graph under the SQL Dashboard now includes all restarts previously categorized as "Write Too Old (multiple)". The former is a now a superset of the latter. The "Write Too Old (multiple)" metric will be removed in a future release.
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
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. |
|
This change is |
There was a problem hiding this comment.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @nvanbenschoten)
-- commits line 7 at r1:
It is never, right?
-- commits line 11 at r1:
Should we open an issue for this?
Separately, should we also open an issue to remove the WriteTooOld field from Transaction proto, now that we don't need to worry about 23.1 compatibility?
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
Previously, arulajmani (Arul Ajmani) wrote…
It is never, right?
In non-mixed version clusters, I believe that's correct.
Should we open an issue for this?
Done: #119413.
Separately, should we also open an issue to remove the
WriteTooOldfield fromTransactionproto, now that we don't need to worry about 23.1 compatibility?
Done: #119414.
There was a problem hiding this comment.
Reviewable status:
Previously, nvanbenschoten (Nathan VanBenschoten) wrote…
Should we open an issue for this?
Done: #119413.
Separately, should we also open an issue to remove the
WriteTooOldfield fromTransactionproto, now that we don't need to worry about 23.1 compatibility?Done: #119414.
Thanks for opening these!
|
Build succeeded: |
WriteTooOld errors were previously counted in either the
txn.restarts.writetoooldmetric or thetxn.restarts.writetoooldmultimetric, depending on when in the lifecycle of a transaction they were thrown. Now that WriteTooOld errors are rarely (never?) deferred, we would like to consolidate these metrics to avoid confusion.As a first step, we now increment
txn.restarts.writetoooldin both cases. In the next release, we can remove thetxn.restarts.writetoooldmultimetric and its presence in the "Transaction Restarts" graph in the SQL Dashboard.Release note (ui change): The "Write Too Old" metric in the "Transaction Restarts" graph under the SQL Dashboard now includes all restarts previously categorized as "Write Too Old (multiple)". The former is a now a superset of the latter. The "Write Too Old (multiple)" metric will be removed in a future release.