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

stats: use leased types in the stats cache #53078

Merged
merged 1 commit into from
Aug 20, 2020

Conversation

rohany
Copy link
Contributor

@rohany rohany commented Aug 19, 2020

This commit switches the stats cache to use leased types rather than
accessing types directly. This was the final user of the direct type
access methods, so that code is removed as well.

Release note: None

@rohany rohany requested review from ajwerner and RaduBerinde August 19, 2020 20:26
@rohany rohany requested a review from a team as a code owner August 19, 2020 20:26
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Reviewed 5 of 7 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner, @RaduBerinde, and @rohany)


pkg/sql/stats/stats_cache.go, line 419 at r1 (raw file):

			// collecting the stats. Changes to types are backwards compatible across
			// versions, so using a newer version of the type metadata here is safe.
			// Additionally, the leased type that is accessed here is guaranteed to

This comment and code seems pretty suspect. You're guaranteed to be able to read the values of the type while you hold the lease and the transaction's read timestamp doesn't get pushed above the lease's deadline. the fact that this transaction isn't tied to the lifetime of the request which read the data seems odd. One thing that would work would be to pass in an hlc timestamp that was used to create the stats (I'm pretty sure they are created at a timestamp), and then to set the transaction timestamp here to that fixed timestamp. Then I think this mostly works. The last remaining problem is that it's not clear to me that we're releasing the reference to the lease that we acquire underneath this new collection.

@rohany
Copy link
Contributor Author

rohany commented Aug 19, 2020

I'm not too keen on even using the lease manager here honestly, I just wanted to get rid of the last user of ResolveTypeDescByID. I might just update this to get the type descriptor directly using a txn.

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.

I think the lease manager is ... fine. I'd just like to see a reasonably timestamp make its way here if we're going to make claims. I feel like given the fact that you can't delete an enum value, I'd be okay if you just say that so long as you find any descriptor, you'll be able to deal with these stats and thus it doesn't matter if it isn't the exact enum descriptor used to collect the stats.

tl;dr if you fix the comment to explain things reasonably I'm on board.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @RaduBerinde and @rohany)

@rohany rohany force-pushed the stats-cache-lease branch 2 times, most recently from 4c4425f to b5af1fe Compare August 20, 2020 17:46
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:

Reviewed 1 of 7 files at r1, 3 of 3 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @RaduBerinde)

This commit switches the stats cache to use leased types rather than
accessing types directly. This was the final user of the direct type
access methods, so that code is removed as well.

Release note: None
@rohany rohany force-pushed the stats-cache-lease branch from b5af1fe to bab8fc2 Compare August 20, 2020 20:24
@rohany
Copy link
Contributor Author

rohany commented Aug 20, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 20, 2020

Build succeeded:

@craig craig bot merged commit 7023c94 into cockroachdb:master Aug 20, 2020
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