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

sstable: refactor ReaderOptions #4269

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

jbowens
Copy link
Collaborator

@jbowens jbowens commented Jan 17, 2025

Add block.ReaderOptions and refactor sstable.ReaderOptions to contain them, and the blob reader to also use block.Reader and block.ReaderOptions.

@jbowens jbowens requested a review from a team as a code owner January 17, 2025 18:55
@jbowens jbowens requested a review from sumeerbhola January 17, 2025 18:55
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@jbowens jbowens 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 12 files reviewed, 1 unresolved discussion (waiting on @sumeerbhola)


sstable/options.go line 120 at r1 (raw file):

	// internal options can only be used from within the pebble package.
	internal sstableinternal.ReaderOptions

I removed this internal field, but I think it's alright to export the CacheOpts field since the referenced struct itself defined in the internal package.

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.

Reviewable status: 0 of 12 files reviewed, 2 unresolved discussions (waiting on @jbowens and @sumeerbhola)


sstable/options.go line 116 at r1 (raw file):

// this method is public, a caller outside the pebble package can't construct a
// value to pass to it.
func (o *ReaderOptions) SetInternalCacheOpts(cacheOpts sstableinternal.CacheOptions) {

[nit] We no longer need this method (we can just set CacheOpts directly now)

Add block.ReaderOptions and refactor sstable.ReaderOptions to contain them.
Refactor the blob reader to use block.Reader and block.ReaderOptions.

Informs cockroachdb#112.
@jbowens jbowens force-pushed the block-readeroptions branch from 4781c70 to b733394 Compare January 17, 2025 21:38
Copy link
Collaborator Author

@jbowens jbowens 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 19 files reviewed, 1 unresolved discussion (waiting on @sumeerbhola)


sstable/options.go line 116 at r1 (raw file):

Previously, RaduBerinde wrote…

[nit] We no longer need this method (we can just set CacheOpts directly now)

Done

@jbowens jbowens merged commit f612150 into cockroachdb:master Jan 17, 2025
23 checks passed
@jbowens jbowens deleted the block-readeroptions branch January 17, 2025 21:53
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