catalog/lease: fix error handling for purgeOldVersions#157131
catalog/lease: fix error handling for purgeOldVersions#157131craig[bot] merged 1 commit intocockroachdb:masterfrom
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. |
|
This change is |
64cff3c to
e86bbde
Compare
spilchen
left a comment
There was a problem hiding this comment.
@spilchen reviewed 1 of 2 files at r1.
Reviewable status:complete! 0 of 0 LGTMs obtained
pkg/sql/catalog/lease/lease.go line 2331 at r1 (raw file):
_ = s.RunAsyncTask(ctx, "refresh-leases", func(ctx context.Context) { defer func() { if err := recover(); err != nil {
Does this code for recover cause us to suppress panics now? Is the behaviour we want? It seems like an orthogonal issue.
Previously, when purging old versions we would acquire a lease on the latest version, which was introduced when we added logic for acquiring leases on the previous version. The logic to acquire the previous version would clear the error, preventing errors from correctly surfacing and causing issues with dropped / offline descriptors. To address this, ensure the acquisition logic for the previous version is only executed if a clean up will occur. This patch also enhances logging for error scenarios where we could fail to acquire new leases. Informs: cockroachdb#156176 Release note: None
e86bbde to
6abab18
Compare
fqazi
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/catalog/lease/lease.go line 2331 at r1 (raw file):
Previously, spilchen wrote…
Does this code for
recovercause us to suppress panics now? Is the behaviour we want? It seems like an orthogonal issue.
Yes, it makes it easier to detect cases where the async task just crashes on us. I updated these changes to include a rethrow of the panic.
spilchen
left a comment
There was a problem hiding this comment.
@spilchen reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
@spilchen TFTR! bors r+ |
Previously, when purging old versions we would acquire a lease on the latest version, which was introduced when we added logic for acquiring leases on the previous version. The logic to acquire the previous version would clear the error, preventing errors from correctly surfacing and causing issues with dropped / offline descriptors. To address this, ensure the acquisition logic for the previous version is only executed if a clean up will occur.
Informs: #156176
Release note: None