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

catkv,*: remove catkv.Direct interface #93611

Merged
merged 9 commits into from
Dec 15, 2022

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Dec 14, 2022

Informs #64089.

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@postamar postamar changed the title [WIP] catkv,*: remove catkv.Direct interface catkv,*: remove catkv.Direct interface Dec 14, 2022
@postamar postamar marked this pull request as ready for review December 14, 2022 19:54
@postamar postamar requested review from a team as code owners December 14, 2022 19:54
@postamar postamar requested review from a team December 14, 2022 19:54
@postamar postamar requested a review from a team as a code owner December 14, 2022 19:54
@postamar postamar requested review from benbardin, stevendanna and ajwerner and removed request for a team, benbardin and stevendanna December 14, 2022 19:54
Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

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

:lgtm: very nice

Reviewed 5 of 5 files at r1, 5 of 5 files at r2, 2 of 2 files at r3, 12 of 12 files at r4, 2 of 2 files at r5, 1 of 1 files at r6, 31 of 31 files at r7, 6 of 6 files at r8, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @postamar)

@postamar
Copy link
Contributor Author

Thanks for the review! CI is unhappy due to an unrelated flaky test.

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2022

Merge conflict.

@postamar
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2022

Build failed:

Marius Posta added 2 commits December 14, 2022 22:27
This commit replaces it with the GetAllFromStorageUnvalidated
method on descs.Collection

Release note: None
This commit transfers the ownership of the node's
catkv.SystemDatabaseCache singleton from the descs.CollectionFactory to
the lease.Manager. It also replaces catkv.Direct usage in the lease
package by catkv.CatalogReader calls.

Release note: None
Marius Posta added 7 commits December 14, 2022 22:27
This commit removes the MaybeGetDescriptorByIDUnvalidated method from
the catkv.Direct interface.

Release note: None
These methods are replaced by new functions in the descs package which
operate on the descs.Collection.

Release note: None
This commit removes dead code.

Release note: None
This can now be replaced by calls to descs.Collection methods.

Informs cockroachdb#64089.

Release note: None
This removes the last usage of uncached catkv.CatalogReader outside of
testing.

Release note: None
This commit fixes a bug in which persisted catalog changes were not
visible in the results of this descs.Collection's method when the
transation already involved a full catalog scan before those changes.

Release note: None
@postamar
Copy link
Contributor Author

The flake wasn't unrelated and in fact the workload test found a legit bug! I'm grateful for it.

@postamar
Copy link
Contributor Author

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2022

Build failed:

@postamar
Copy link
Contributor Author

This time it really is an unrelated flake.

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 15, 2022

Build succeeded:

@craig craig bot merged commit 53b1dea into cockroachdb:master Dec 15, 2022
@postamar postamar deleted the bye-bye-direct branch December 15, 2022 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants