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: consolidate reader/writer options #3736

Merged

Conversation

RaduBerinde
Copy link
Member

sstable: remove rawTombstonesOpt

This option does nothing - the flag it sets is not used anymore.

sstable: move FilterMetricsTracker to ReaderOptions

There is no good reason for this option to be a separate
ReaderOption.

sstable: add Writer method to set snapshot pinned stats

We have a private hook that can be used to get a direct reference to
the properties in the writer. We only use this to set the snapshot
pinned stats. We replace this awkward mechanism with just a simple
method to set these stats. There is little reason to go through hoops
to disallow usage of such a method.

sstable: consolidate reader/writer options

There are currently three mechanisms to configure sstable.Reader or
Writer:

  • most settings use ReaderOptions/WriterOptions
  • some settings use ReaderOption/WriterOption; some of these are
    kept private using a hook through the private package
  • some settings call a function that modifies a Writer, also through
    the private package

This commit moves all settings to ReaderOption/WriterOption
structs. Settings which are not allowed to be used outside Pebble are
defined in sstableinternal, which prevents outside callers from
constructing values for them.

sstable: don't store ReaderOptions in Reader

We keep Readers in the table cache so we don't want their size to be
unnecessarily large. We reduce the size by not storing the full
ReaderOptions inside the Reader; many of the fields are only used
inside NewReader.

@RaduBerinde RaduBerinde requested a review from a team as a code owner July 8, 2024 03:58
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RaduBerinde RaduBerinde removed the request for review from aadityasondhi July 8, 2024 03:59
Copy link
Collaborator

@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.

:lgtm:

Reviewed 4 of 4 files at r1, 6 of 6 files at r2, 3 of 3 files at r3, 22 of 33 files at r4.
Reviewable status: 22 of 35 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @RaduBerinde)


internal/sstableinternal/options.go line 30 at r4 (raw file):

	// Reader.NewRangeDelIter() should not be fragmented. Used by debug tools to
	// get a raw view of the tombstones contained in an sstable.
	RawTombstones bool

this was already deleted in a previous commit, right?


internal/sstableinternal/options.go line 39 at r4 (raw file):

// tools which may be used on multiple databases configured with different
// comparers.
type Comparers map[string]*base.Comparer

should Comparers and Mergers actually be private? It seems like a reasonable thing a sstable reader may want to configure

Copy link
Member Author

@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: 22 of 35 files reviewed, 2 unresolved discussions (waiting on @itsbilal and @jbowens)


internal/sstableinternal/options.go line 39 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

should Comparers and Mergers actually be private? It seems like a reasonable thing a sstable reader may want to configure

I guess not.. I did that because they were not in options already. I will move them.

By the way, why do we care about the merger at all? We don't seem to be using it anywhere

This option does nothing - the flag it sets is not used anymore.
There is no good reason for this option to be a separate
`ReaderOption`.
We have a private hook that can be used to get a direct reference to
the properties in the writer. We only use this to set the snapshot
pinned stats. We replace this awkward mechanism with just a simple
method to set these stats. There is little reason to go through hoops
to disallow usage of such a method.
There are currently three mechanisms to configure sstable.Reader or
Writer:
 - most settings use `ReaderOptions`/`WriterOptions`
 - some settings use `ReaderOption`/`WriterOption`; some of these are
   kept private using a hook through the private package
 - some settings call a function that modifies a Writer, also through
   the private package

This commit moves all settings to `ReaderOption`/`WriterOption`
structs. Settings which are not allowed to be used outside Pebble are
defined in `sstableinternal`, which prevents outside callers from
constructing values for them.
We keep Readers in the table cache so we don't want their size to be
unnecessarily large. We reduce the size by not storing the full
`ReaderOptions` inside the `Reader`; many of the fields are only used
inside `NewReader`.
Copy link
Member Author

@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.

TFTR!

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


internal/sstableinternal/options.go line 30 at r4 (raw file):

Previously, jbowens (Jackson Owens) wrote…

this was already deleted in a previous commit, right?

Oops, fixed.


internal/sstableinternal/options.go line 39 at r4 (raw file):

Previously, RaduBerinde wrote…

I guess not.. I did that because they were not in options already. I will move them.

By the way, why do we care about the merger at all? We don't seem to be using it anywhere

Done.

@RaduBerinde RaduBerinde merged commit 26fe5a0 into cockroachdb:master Jul 9, 2024
11 checks passed
@RaduBerinde RaduBerinde deleted the sstable-options-refactor branch July 9, 2024 18:56
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.

None yet

3 participants