Skip to content

Conversation

ggevay
Copy link
Contributor

@ggevay ggevay commented Aug 15, 2025

least_valid_read doesn't need access to any Catalog state; it can be done simply on ReadHolds. This PR modifies all call sites of Catalog::least_valid_read to instead call ReadHolds::least_valid_read directly, and deletes Catalog::least_valid_read. This makes the code at these call sites easier to use from outside the Coordinator, and thus works towards the Small Coordinator vision, specifically https://github.com/MaterializeInc/database-issues/issues/9593.

Motivation

  • This PR refactors existing code.

Tips for reviewer

Checklist

  • This PR has adequate test coverage / QA involvement has been duly considered. (trigger-ci for additional test/nightly runs)
  • This PR has an associated up-to-date design doc, is a design doc (template), or is sufficiently small to not require a design.
  • If this PR evolves an existing $T ⇔ Proto$T mapping (possibly in a backwards-incompatible way), then it is tagged with a T-proto label.
  • If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label (example).
  • If this PR includes major user-facing behavior changes, I have pinged the relevant PM to schedule a changelog post.

@ggevay ggevay requested a review from aljoscha August 15, 2025 14:07
@ggevay ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Aug 15, 2025
@ggevay ggevay requested review from a team as code owners August 15, 2025 14:07
Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Yeah, a lot of these are cleanup opportunities that came from earlier refactors that we then never did. 🙌

@ggevay ggevay force-pushed the least_valid_read-no-catalog branch from 589e705 to e7f8eb3 Compare August 20, 2025 09:03
@ggevay ggevay enabled auto-merge August 20, 2025 09:04
@ggevay ggevay merged commit 2442dbe into MaterializeInc:main Aug 20, 2025
127 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ADAPTER Topics related to the ADAPTER layer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants