sql: TestReacquireLeaseOnRestart fix #137628
Conversation
![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 |
| @@ -172,6 +172,7 @@ func (t *descriptorState) upsertLeaseLocked( | |||
| } | |||
| if session != nil { | |||
| s.mu.lease.sessionID = session.ID().UnsafeBytes() | |||
| toRelease = nil | |||
There was a problem hiding this comment.
Add a comment stating why toRelease is being cleared here.
// When using session based leasing, if we end up acquiring the same lease again nothing needs to be cleaned up or updated. This is because the system.lease table does not store any expiry inside the table.
There was a problem hiding this comment.
Thanks Faizan! I added the comment.
| @@ -821,7 +821,7 @@ func purgeOldVersions( | |||
| // if we hit this case. Note: This scenario is impossible to hit in the real | |||
| // world since the lease duration is never set to 0. | |||
| desc, _, err := t.findForTimestamp(ctx, m.storage.clock.Now()) | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive || errors.Is(err, errRenewLease) { | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive { | |||
There was a problem hiding this comment.
Can you try reverting this part of the change and check if the test is stable? I think it's causing CI failures
There was a problem hiding this comment.
Please update the comment above to remove the reference to this error. We no longer have the migration scenario any more.
There was a problem hiding this comment.
Thanks Faizan! I removed the comment.
Dedej-Bergin
force-pushed
the
reacquire-lease
branch
from
95df273 to
4cd8870
Compare
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @Dedej-Bergin)
| @@ -821,7 +821,7 @@ func purgeOldVersions( | |||
| // if we hit this case. Note: This scenario is impossible to hit in the real | |||
| // world since the lease duration is never set to 0. | |||
| desc, _, err := t.findForTimestamp(ctx, m.storage.clock.Now()) | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive || errors.Is(err, errRenewLease) { | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive { | |||
There was a problem hiding this comment.
Please update the comment above to remove the reference to this error. We no longer have the migration scenario any more.
Dedej-Bergin
force-pushed
the
reacquire-lease
branch
from
4cd8870 to
ab44d86
Compare
There was a problem hiding this comment.
Reviewable status:
| @@ -172,6 +172,7 @@ func (t *descriptorState) upsertLeaseLocked( | |||
| } | |||
| if session != nil { | |||
| s.mu.lease.sessionID = session.ID().UnsafeBytes() | |||
| toRelease = nil | |||
There was a problem hiding this comment.
Thanks Faizan! I added the comment.
| @@ -821,7 +821,7 @@ func purgeOldVersions( | |||
| // if we hit this case. Note: This scenario is impossible to hit in the real | |||
| // world since the lease duration is never set to 0. | |||
| desc, _, err := t.findForTimestamp(ctx, m.storage.clock.Now()) | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive || errors.Is(err, errRenewLease) { | |||
| if isInactive := catalog.HasInactiveDescriptorError(err); err == nil || isInactive { | |||
There was a problem hiding this comment.
Thanks Faizan! I removed the comment.
rafiss
added
backport-24.2.x
Previously we had an extra condition in an if statement which caused this failure. This was the condition `errors.Is(err, errRenewLease)` it is now removed and we now set toRelease to nil when `session != nil` Fixes: cockroachdb#129421 Release note: none
Dedej-Bergin
force-pushed
the
reacquire-lease
branch
from
ab44d86 to
ac417b0
Compare
Previously we had an extra condition in an if statement which caused this failure. This was the condition
errors.Is(err, errRenewLease)it is now removed and we now set toRelease to nil whensession != nilFixes: #129421
Fixes: #130491
Release note: none