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

[sinks] Set with_snapshot=false even during history reduction #31490

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bkirwi
Copy link
Contributor

@bkirwi bkirwi commented Feb 13, 2025

Motivation

In #31152, we added an optimization for with_snapshot that kicks in when environmentd restarts, such as during a deploy. However, if only clusterd restarts, we will use the sink definition cached in the history implementation... and if the first run of the sink needed the snapshot, then we'll fetch the snapshot after a restart too.

This PR also applies the optimization in the history, so the snapshot can be skipped when even the first run of a sink is restarted. I feel less confident in this change, since the "chain of reasoning" that implies it is safe is a bit more complex. But it may be worthwhile to avoid the operational surprise.

Happy for feedback on this point however!

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 bkirwi marked this pull request as ready for review February 18, 2025 15:18
@bkirwi bkirwi requested a review from a team as a code owner February 18, 2025 15:18
@bkirwi
Copy link
Contributor Author

bkirwi commented Feb 18, 2025

Nightlies - the first run was pretty red, but it seems to be unrelated breakage in main... rerunning the failed tests after a rebase seems greener.

@bkirwi bkirwi requested review from aljoscha and petrosagg February 20, 2025 15:15
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.

1 participant