Skip to content

[persist] Lease cleanup #32459

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

[persist] Lease cleanup #32459

wants to merge 3 commits into from

Conversation

bkirwi
Copy link
Contributor

@bkirwi bkirwi commented May 9, 2025 •
edited
Loading

This bundles together a few related cleanups for the leased part:

  • We had a few proto types that were only useful for short-lived exchanges in the source. We encode those with serde now. (While continuing to encode long-lived data like the hollow batch as proto.)
  • As part of the batch fetching, we created leased parts that didn't actually have leases attached, then consumed them immediately. This switches the fetching code to use the exchangeable part instead and makes leases required in all other cases.

Motivation

Previously-unspecified cleanup.

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.

@bkirwi
More serde exchangeable part
d516d58 
It's currently possible, but a bit odd, to have an exchangeable part
that doesn't have a lease attached. This switches us to using the
"exchangeable" part directly, instead of inventing and immediately
dropping the lease.
bkirwi added 2 commits May 12, 2025 15:49
@bkirwi
Bundle the seqno and the lease together
276e078
@bkirwi
Leased parts always have leases now
e028eb0 
The only case where this wasn't true was exchanging over a network, and
we no longer use leased part for that.
@bkirwi bkirwi marked this pull request as ready for review May 12, 2025 22:22
@bkirwi bkirwi requested review from a team as code owners May 12, 2025 22:22
@bkirwi
Copy link
Contributor Author

bkirwi commented May 12, 2025

An earlier version of this branch had some nightly failures in one of the alter sink tests. This test is timing-dependent, so it's plausible that this was either a flake or a timing change. (We'd expect this branch to be timing-neutral or perhaps a very mild improvement.) I reran those tests after a rebase and they came back green. 🤷

ParkMyCar
ParkMyCar approved these changes May 13, 2025
View reviewed changes
Copy link
Member

@ParkMyCar ParkMyCar left a comment

Choose a reason for hiding this comment

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

Looks great! I like the notion of proto == durable, serde == transient

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ParkMyCar ParkMyCar ParkMyCar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bkirwi @ParkMyCar