Skip to content

Conversation

sumeerbhola
Copy link
Collaborator

@sumeerbhola sumeerbhola commented Aug 8, 2025

initFileOpts only contains InitFileReadStats, which allows the caller to provide stats collected by iterators. Reads done when creating a Reader specifically because an iterator was needed on that file, will now be captured in the corresponding iterator stats.

This reduces the uncertainty when we see slowness in traces around iteration, and are unsure whether it happened prior to operations on the iterator. We continue to have some unaccounted latency:

  • Footer reads are not included in the stats.
  • File cache entry initialization is shared when there are concurrent attempts to create the entry, and the latency (via iterator stats) is only accounted in the one that did the Reader creation.

initFileOpts are also plumbed to valblk.blockProviderWhenClosed, when
it needs to open a sstable.Reader. As part of this change, we also
fix the missing stats for valblk.ReadValueBlockExternal, and the
missing category stats in general when reading value blocks.

@sumeerbhola sumeerbhola requested a review from a team as a code owner August 8, 2025 15:59
@sumeerbhola sumeerbhola requested a review from RaduBerinde August 8, 2025 15:59
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

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

also related to https://cockroachlabs.slack.com/archives/C04U1BTF8/p1754564947355449

Reviewable status: 0 of 15 files reviewed, all discussions resolved (waiting on @RaduBerinde)

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 13 of 15 files at r1, all commit messages.
Reviewable status: 13 of 15 files reviewed, 2 unresolved discussions (waiting on @RaduBerinde)


file_cache.go line 283 at r1 (raw file):

		fileType: ftyp,
	}
	valRef, err := h.fileCache.c.FindOrCreate(ctx, key, initFileOpts{})

relates to my other question, but should we collect these stats for blob file opens too--or leave a TODO to do so?


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

// and we are able to capture these initialization stats in the iterator
// stats.
type InitFileReadStats struct {

should we put this in the block package and collect it for blob file opens too?

initFileOpts only contains InitFileReadStats, which allows the caller to
provide stats collected by iterators. Reads done when creating a Reader
specifically because an iterator was needed on that file, will now be
captured in the corresponding iterator stats.

This reduces the uncertainty when we see slowness in traces around
iteration, and are unsure whether it happened prior to operations on the
iterator. We continue to have some unaccounted latency:
- Footer reads are not included in the stats.
- File cache entry initialization is shared when there are concurrent
  attempts to create the entry, and the latency (via iterator stats) is
  only accounted in the one that did the Reader creation.

initFileOpts are also plumbed to valblk.blockProviderWhenClosed, when
it needs to open a sstable.Reader. As part of this change, we also
fix the missing stats for valblk.ReadValueBlockExternal, and the
missing category stats in general when reading value blocks.
Copy link
Collaborator Author

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


file_cache.go line 283 at r1 (raw file):

Previously, jbowens (Jackson Owens) wrote…

relates to my other question, but should we collect these stats for blob file opens too--or leave a TODO to do so?

I had the TODO earlier about collecting stats for blob file opens, but looking at it again, that was for the last step and the first step, but there is no reason to not put in most of the plumbing now, and just leave the last step as a TODO.
Fixed now.

Also made some other plumbing improvements in valblk related to reading blocks.


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

Previously, jbowens (Jackson Owens) wrote…

should we put this in the block package and collect it for blob file opens too?

Done

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_strong:

Reviewed 11 of 11 files at r2, all commit messages.
Reviewable status: 20 of 22 files reviewed, 2 unresolved discussions (waiting on @RaduBerinde)

@sumeerbhola sumeerbhola merged commit 6570ae2 into cockroachdb:master Aug 19, 2025
8 checks passed
@sumeerbhola sumeerbhola deleted the reader_iter_stats branch August 19, 2025 15:19
sumeerbhola added a commit to sumeerbhola/pebble that referenced this pull request Aug 19, 2025
This test broke in cockroachdb#5164, even
though CI was green.
sumeerbhola added a commit that referenced this pull request Aug 19, 2025
This test broke in #5164, even
though CI was green.
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