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

storage: CheckSSTConflicts: fix instance of iterator mismatch #99695

Merged

Conversation

itsbilal
Copy link
Contributor

@itsbilal itsbilal commented Mar 27, 2023

Previously, in one case, we'd let the sst iterator advance ahead of the engine iterator, which violates an invariant in this function. This change fixes that case.

Fixes #99566.
Fixes #99010.

Epic: none

Release note: None

@itsbilal itsbilal self-assigned this Mar 27, 2023
@itsbilal itsbilal requested review from a team as code owners March 27, 2023 17:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@itsbilal
Copy link
Contributor Author

Stressed the seeds in both #99010 and #99566 and they both pass with this change.

Previously, in one case, we'd let the sst iterator advance
ahead of the engine iterator, which violates an invariant
in this function. This change fixes that case.

Fixes cockroachdb#99566.
Fixes cockroachdb#99010.

Epic: none

Release note: None
@RaduBerinde
Copy link
Member

pkg/storage/sst.go line 1140 at r1 (raw file):

						sstIter.SeekGE(MVCCKey{Key: extIter.UnsafeKey().Key})
						sstOK, sstErr = sstIter.Valid()
						extIter.SeekGE(MVCCKey{Key: sstIter.UnsafeKey().Key})

Do we know for sure that sstOK will be true now? In any case, would it make more sense to remove the extChangedKeys in the condition below and let it fall through?

@itsbilal itsbilal force-pushed the checksstconflicts-kvnemesis-newest-fix branch from ea2df2a to 49507f3 Compare March 27, 2023 17:30
@itsbilal
Copy link
Contributor Author

pkg/storage/sst.go line 1140 at r1 (raw file):

Previously, RaduBerinde wrote…

Do we know for sure that sstOK will be true now? In any case, would it make more sense to remove the extChangedKeys in the condition below and let it fall through?

I had just realized I pushed an older commit. Fixed this now.

@RaduBerinde
Copy link
Member

pkg/storage/sst.go line 1140 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

I had just realized I pushed an older commit. Fixed this now.

Should we also check extOK?

@itsbilal
Copy link
Contributor Author

pkg/storage/sst.go line 1140 at r1 (raw file):

Previously, RaduBerinde wrote…

Should we also check extOK?

We don't need to, because !extChangedKeys guarantees extOK.

Copy link
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @erikgrinaker and @jbowens)


pkg/storage/sst.go line 1140 at r1 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

We don't need to, because !extChangedKeys guarantees extOK.

Ah, indeed.

@itsbilal itsbilal added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 27, 2023
@itsbilal
Copy link
Contributor Author

TFTR!

bors r=RaduBerinde

@craig
Copy link
Contributor

craig bot commented Mar 27, 2023

Build failed:

@itsbilal
Copy link
Contributor Author

Acceptance test timeout.

bors retry

@craig
Copy link
Contributor

craig bot commented Mar 27, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 27, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Mar 28, 2023

Build succeeded:

@craig craig bot merged commit 143b63a into cockroachdb:master Mar 28, 2023
@itsbilal itsbilal deleted the checksstconflicts-kvnemesis-newest-fix branch March 28, 2023 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kv/kvnemesis: TestKVNemesisSingleNode failed kv/kvnemesis: TestKVNemesisSingleNode failed
3 participants