kvserver: improve logs during adminScatter #147359
Conversation
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=95b789b1d594ac6b74f377aa7555d8d98d63240d&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! 1 of 0 LGTMs obtained (waiting on @wenyihu6)
pkg/kv/kvserver/replica_command.go line 4223 at r1 (raw file):
// The replica can not be processed, so skip it. log.Warningf(ctx, "cannot process range (%v) due to %v at %dth attempt",
nit: the dth attempt here and below will be odd to read for attempts < 4 (1th/2th/3th). Consider reformatting.
Previously, the logs in adminScatter included the message "failed to scatter", which could be misleading, as these are not always true failures and depend on context. For example, hitting a terminating error during a retry after rebalancing the leaseholder away doesn’t necessarily indicate a failure without further assessment. This patch updates the log message to avoid confusion for users searching in logs. In addition, it also changes the logging call from log.Eventf to log.Infof, since Eventf was a mistake and not included in logs by default. Epic: none Release note: none
There was a problem hiding this comment.
Reviewable status:
pkg/kv/kvserver/replica_command.go line 4223 at r1 (raw file):
Previously, kvoli (Austen) wrote…
nit: the
dthattempt here and below will be odd to read for attempts < 4 (1th/2th/3th). Consider reformatting.
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
TFTR! bors r=kvoli |
Previously, the logs in adminScatter included the message "failed to scatter",
which could be misleading, as these are not always true failures and depend on
context. For example, hitting a terminating error during a retry after
rebalancing the leaseholder away doesn’t necessarily indicate a failure without
further assessment. This patch updates the log message to avoid confusion for
users searching in logs. In addition, it also changes the logging call from
log.Eventf to log.Infof, since Eventf was a mistake and not included in logs
by default.
Epic: none
Release note: none