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
storage: correct terminology in log line for disk stalls #114746
Conversation
Previously, we'd use the term "file write stall" in the log line for a disk stall, and not use the term "disk stall" which is what metrics and documentation refers to this event as. Furthermore, "write stalls" are a different, unrelated kind of stall in Pebble. This change addresses this by changing the wording to refer to this event as a "disk stall" in the log. Epic: none Release note (ops change): Updates the error message in case of stalled disks to use the appropriate term for that event ("disk stall"), which is also what metrics/dashboards refer to that event as.
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.
I think this language was deliberately changed to "file write stall" after we once saw a single large (megabytes) write take long enough to trigger the disk stall detector. I do think that that change confused more than it clarified, so this
With hindsight, I wish we named "write stalls" as "engine stalls," and "disk stalls" as "IO stalls."
Reviewable status: complete! 1 of 0 LGTMs obtained
@jbowens ah okay, that's good to know. I didn't realize we've litigated this already, I just guessed it was a mistake from the last time we touched this code. The other option is "file write disk stall". I just want a more direct connection between this event and the term "disk stall" but it's not the most pressing thing either. I don't mind closing this PR |
@itsbilal Idk, I say merge it. It's probably most important that the end user understand which of the two concepts they're observing: a disk stall or a write stall. |
Sounds good. Thanks! bors r=jbowens |
Build succeeded: |
Previously, we'd use the term "file write stall" in the log line for a disk stall, and not use the term "disk stall" which is what metrics and documentation refers to this event as. Furthermore, "write stalls" are a different, unrelated kind of stall in Pebble.
This change addresses this by changing the wording to refer to this event as a "disk stall" in the log.
Epic: none
Release note (ops change): Updates the error message in case of stalled disks to use the appropriate term for that event ("disk stall"), which is also what metrics/dashboards refer to that event as.