[Oneshot Sources] Fix bad assert in storage worker reconciliation#35556
Conversation
|
Thanks for opening this PR! Here are a few tips to help make the review process smooth for everyone. PR title guidelines
Pre-merge checklist
|
|
started a nightly to check that the error doesn't recur: https://buildkite.com/materialize/nightly/builds/15770/steps/canvas |
| mz_ore::soft_assert_or_log!( | ||
| !created && dropped, | ||
| "dropped non-existent oneshot source" | ||
| !(!to_create && to_drop), |
There was a problem hiding this comment.
I was waffling about whether this should be:
!(!to_create && to_drop)
or
to_create || !to_drop
I figured the first option communicates the intent better - we want to ensure that reconciliation is not trying to drop any ingestions that it did not create - as it should have created anything that it wants to exist in the first place.
but, assert_false(!to_create && to_drop) would be the best option, but alas we do not have mz_ore::soft_assert_false_or_log!
There was a problem hiding this comment.
Nice!
The one you chose looks good to me. Here's something to add to your waffling - you can also just add an intermediate variable for readability.
let drop_without_create = to_drop && !to_create;
mz_ore::soft_assert_or_log(
!drop_without_create,
...
);
I think for a quick fix this is good, but we should definitely take a broader accounting of the flow. I started looking at how we reconcile, and as far as I can tell, the storage controller wouldn't send a CancelOneshotIngestion.
If a replica disconnects (either network issue, or restart), the storage controller should call add_replica(), which first calls reduce on the command history. The result of the reduce doesn't store any CancelOnceshotIngestion in the history, it removes RunOneshotIngestion commands from the history.
It looks like oneshot ingestions are only kept in memory in envd, so if envd restarts, it also wouldn't send cancellations.
patrickwwbutler
merged commit 6904a6b
into
MaterializeInc:main
…5556) ### Motivation fixes MaterializeInc/database-issues#11255 ### Description Fixes a previous mistake in writing an assert - the assert was written as though it panics when the condition is true, so this inverts the condition, and improves variable names and error message to better clarify the intent.
2 participants
Motivation
fixes https://github.com/MaterializeInc/database-issues/issues/11255
Description
Fixes a previous mistake in writing an assert - the assert was written as though it panics when the condition is true, so this inverts the condition, and improves variable names and error message to better clarify the intent.