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

db: refactor disk usage estimate using range annotations #3848

Merged

Conversation

anish-shanbhag
Copy link
Contributor

@anish-shanbhag anish-shanbhag commented Aug 13, 2024

This change updates db.EstimateDiskUsage to use range annotations to
estimate the disk usage of a key range. This should improve the
performance of repeated disk usage estimates for similar or identical
key ranges.

At the Cockroach layer we use db.EstimateDiskUsage in a few places,
most notably when computing MVCC span stats.

Informs: #3793

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@anish-shanbhag anish-shanbhag changed the title Disk usage annotator db: refactor disk usage estimate using range annotations Aug 13, 2024
@anish-shanbhag anish-shanbhag force-pushed the disk-usage-annotator branch 3 times, most recently from ef1464c to 4c6c47b Compare August 14, 2024 15:59
@anish-shanbhag anish-shanbhag marked this pull request as ready for review August 21, 2024 15:36
@anish-shanbhag anish-shanbhag requested a review from a team as a code owner August 21, 2024 15:36
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: Nice!

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @anish-shanbhag, @itsbilal, and @jbowens)


db.go line 508 at r1 (raw file):

		// should be protected from concurrent access.
		annotators struct {
			totalSizeAnnotator    *manifest.Annotator[uint64]

[nit] Annotator is redundant given that these are inside annotators.


internal/manifest/annotator.go line 281 at r1 (raw file):

// for all files within the given Version which are within the range
// defined by bounds.
func (a *Annotator[T]) VersionRangeAnnotation(v *Version, bounds base.UserKeyBounds) *T {

[nit] Why not return T?

This change updates `db.EstimateDiskUsage` to use range annotations to
estimate the disk usage of a key range. This should improve the
performance of repeated disk usage estimates for similar or identical
key ranges.

At the Cockroach layer we use `db.EstimateDiskUsage` in a few places,
most notably when [computing MVCC span stats](https:/cockroachdb/cockroach/blob/master/pkg/server/span_stats_server.go#L217).

Informs: cockroachdb#3793
Copy link
Contributor Author

@anish-shanbhag anish-shanbhag left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)


db.go line 508 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Annotator is redundant given that these are inside annotators.

Thanks, fixed this.


internal/manifest/annotator.go line 281 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] Why not return T?

This is because of the convention I went with in #3760 where annotators always return a pointer type since a pointer is necessary for the internal caching behavior. Some annotators have a T = fileMetadata and in this case we would want to return a *fileMetadata instead of fileMetadata.

Copy link
Contributor Author

@anish-shanbhag anish-shanbhag left a comment

Choose a reason for hiding this comment

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

TFTR!

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @itsbilal, @jbowens, and @RaduBerinde)

@anish-shanbhag anish-shanbhag merged commit 7b047cd into cockroachdb:master Aug 27, 2024
11 checks passed
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