Skip to content

Conversation

@ggevay
Copy link
Contributor

@ggevay ggevay commented Nov 26, 2025

In my earlier slow-path peek and COPY TO PRs I conveniently forgot about the dependencies dropping during sequencing problem. Nightly now reminded me of this with some dataflow creation cannot fail panics.

This PR makes us gracefully handle this problem, with showing AdapterError::ConcurrentDependencyDrop to the user.

Nightly (subset): https://buildkite.com/materialize/nightly/builds/14259

(Temporarily turns on enable_frontend_peek_sequencing in CI. I'll remove this commit before merging.)

@ggevay ggevay added the A-ADAPTER Topics related to the ADAPTER layer label Nov 26, 2025
@ggevay ggevay force-pushed the frontend-peek-dataflow-creation-panic branch from c10dbe6 to 7a0719b Compare November 26, 2025 18:39
.unwrap_or_terminate("cannot fail to create dataflows");
.map_err(
AdapterError::concurrent_dependency_drop_from_dataflow_creation_error,
)?;
Copy link
Contributor Author

@ggevay ggevay Nov 26, 2025

Choose a reason for hiding this comment

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

This change can possibly affect the old peek sequencing, but I hope that's ok.

@ggevay ggevay force-pushed the frontend-peek-dataflow-creation-panic branch from 7a0719b to 615c9f8 Compare November 26, 2025 18:49
@ggevay ggevay requested a review from teskje November 26, 2025 18:53
@ggevay ggevay marked this pull request as ready for review November 26, 2025 18:53
@ggevay ggevay requested a review from a team as a code owner November 26, 2025 18:53
@ggevay ggevay requested review from SangJunBak and removed request for SangJunBak November 26, 2025 18:53
@ggevay ggevay force-pushed the frontend-peek-dataflow-creation-panic branch from 615c9f8 to e5e43fa Compare November 26, 2025 19:03
@def-
Copy link
Contributor

def- commented Nov 27, 2025

I think this fixes the issue we just saw in Nightly on main?
parallel-workload-materialized-1 | thread 'coordinator' panicked at src/adapter/src/util.rs:262:23: cannot fail to create dataflows: SinceViolation(User(340))
Seen in Parallel Workload (rename)

@ggevay
Copy link
Contributor Author

ggevay commented Nov 27, 2025

Hmm, it doesn't fix SinceViolation. Specifically for SinceViolation, it makes that from a panic into an InternalError, but that's still not good. I'll look into this more.

Copy link
Contributor

@teskje teskje left a comment

Choose a reason for hiding this comment

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

LGTM

@ggevay
Copy link
Contributor Author

ggevay commented Nov 27, 2025

I'll merge this now, because it should improve the situation, but the SinceViolation is still a mystery. I'll investigate that more. It happened in a slow-path peek's create_dataflow's validation, and I don't understand how:

  • The read_holds variable in try_frontend_peek_inner is alive until the end of try_frontend_peek_inner.
  • ExecuteSlowPathPeek doesn't return before implement_peek_plan calls create_dataflow.

(I've checked that Parallel Workload has an ignore for ConcurrentDependencyDrop's error msg, but not for the internal error. So, some of the Nightly failures will go away after merging this, but not those that are due to SinceViolation.)

@ggevay ggevay force-pushed the frontend-peek-dataflow-creation-panic branch from e5e43fa to 2f54151 Compare November 27, 2025 09:54
@ggevay ggevay enabled auto-merge November 27, 2025 09:54
@ggevay ggevay merged commit a210fc1 into MaterializeInc:main Nov 27, 2025
125 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.

3 participants