Skip to content
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

backupccl: don't disable leases in test #92481

Merged
merged 1 commit into from
Nov 29, 2022

Conversation

stevendanna
Copy link
Collaborator

@stevendanna stevendanna commented Nov 25, 2022

399e56b introduced a bounded staleness read into the migration machinery.

When lease.TestingDisableTableLeases() has been set, this bounded staleness read encounters an error:

testcluster.go:384: migration-job-find-already-completed: cannot
set fixed timestamp, txn "sql txn" meta={id=f4142488 key=/Min
pri=0.01688073 epo=0 ts=1669334862.467371575,0
min=1669334862.467371575,0 seq=0} lock=false stat=PENDING
rts=1669334862.467371575,0 wto=false gul=1669334862.967371575,0
already performed reads

I believe that the read that was already performed in this case was the descriptor lookup. Then, when we go to execute the select, we attempt to SetFixedTimestamp in txn.NegotiateAndSend.

When the testing isn't in use, on its face it looks like we don't hit this case because we don't allow a fallback to a store lookup:

if err != nil {
_, isBoundedStalenessRead := txn.(*maxTimestampBoundDeadlineHolder)
// Read the descriptor from the store in the face of some specific errors
// because of a known limitation of AcquireByName. See the known
// limitations of AcquireByName for details.
// Note we never should read from store during a bounded staleness read,
// as it is safe to return the schema as non-existent.
if shouldReadFromStore =
!isBoundedStalenessRead && ((catalog.HasInactiveDescriptorError(err) &&
errors.Is(err, catalog.ErrDescriptorDropped)) ||
errors.Is(err, catalog.ErrDescriptorNotFound)); shouldReadFromStore {
return nil, true, nil
}
// Lease acquisition failed with some other error. This we don't
// know how to deal with, so propagate the error.
return nil, false, err
}

But, when TestingDisableTableLeases is set, we skip right to the store lookup:

if flags.AvoidLeased || flags.RequireMutable || lease.TestingTableLeasesAreDisabled() {
return continueLookups, descpb.InvalidID, nil
}

I haven't looked into why lease.TestingDisableTableLeases() was in place in the past, but it is no longer used in any other backup tests and isn't likely needed here.

Fixes #92432
Fixes #92433
Fixes #92434
Fixes #92435

Release note: None

399e56b introduced a bounded
staleness read into the migration machinery.

When `lease.TestingDisableTableLeases()` has been set, this bounded
staleness read encounters an error:

    testcluster.go:384: migration-job-find-already-completed: cannot
    set fixed timestamp, txn "sql txn" meta={id=f4142488 key=/Min
    pri=0.01688073 epo=0 ts=1669334862.467371575,0
    min=1669334862.467371575,0 seq=0} lock=false stat=PENDING
    rts=1669334862.467371575,0 wto=false gul=1669334862.967371575,0
    already performed reads

I believe that the read that was already performed in this case was
the descriptor lookup. Then, when we go to execute the select, we
attempt to SetFixedTimestamp in txn.NegotiateAndSend.

When the testing isn't in use, on its face it looks like we don't hit
this case because we don't allow a fallback to a store lookup:

https://github.com/cockroachdb/cockroach/blob/b5be006bedd7d3cedc3fb3d2248df168e3d64be2/pkg/sql/catalog/descs/leased_descriptors.go#L143-L155

But, when TestingDsiableTableLeases is set, we skip right to the store
lookup:

https://github.com/cockroachdb/cockroach/blob/b5be006bedd7d3cedc3fb3d2248df168e3d64be2/pkg/sql/catalog/descs/descriptor.go#L489-L491

I haven't looked into why lease.TestingDisableTableLeases() was in
place in the past, but it is no longer used in any other backup tests
and isn't likely needed here.

Fixes cockroachdb#92432
Fixes cockroachdb#92433
Fixes cockroachdb#92434
Fixes cockroachdb#92435

Release note: None
@stevendanna stevendanna requested a review from a team as a code owner November 25, 2022 08:39
@stevendanna stevendanna requested review from benbardin and removed request for a team November 25, 2022 08:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@adityamaru adityamaru self-requested a review November 25, 2022 09:10
@stevendanna
Copy link
Collaborator Author

bors r=adityamaru

@craig
Copy link
Contributor

craig bot commented Nov 25, 2022

Build failed:

@stevendanna
Copy link
Collaborator Author

Build failures were #92485 and #92493

@stevendanna
Copy link
Collaborator Author

bors retry

@craig
Copy link
Contributor

craig bot commented Nov 25, 2022

Build failed:

@stevendanna
Copy link
Collaborator Author

bors retry

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Nov 29, 2022

Build succeeded:

@craig craig bot merged commit a16106e into cockroachdb:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants