Fix serve exiting prematurely
#3562
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dominiklohmann
force-pushed
the
topic/serve-premature-exit
branch
from
4eb3df9 to
a4d6680
Compare
lava
approved these changes
Oct 9, 2023
dominiklohmann
force-pushed
the
topic/serve-premature-exit
branch
from
b4f7863 to
a2af8bc
Compare
Pipelines ending with `serve` should wait until all events are fetched from the corresponding `/serve` endpoint. This was not always the case because of an incorrectly set scope in the operator. Fixing that scope, however, led to the operator deadlocking the node under some circumstances. This revealed a bug at the very core of CAF's scheduling logic, which incorrectly blocked not just ordinary but also system messages from arriving when a request response was awaited. I got that bug fixed upstream and backported the fix to our bundled fork of CAF 0.18. That in turn revealead us relying on the broken behavior in our execution nodes accidentally. I've included a fix for that as well.
We will fix this in a follow-up in the near future; in the meantime, the comment in the file explains why the tests are disabled.
dominiklohmann
force-pushed
the
topic/serve-premature-exit
branch
from
a2af8bc to
0b9c3d9
Compare
dominiklohmann
added a commit
that referenced
this pull request
Oct 26, 2023
This is five fixes in one PR: 1. Predicate pushdown did not work in v4.3; this supersedes my previous attempt in #3590 2. The catalog expected expressions to be already normalized and validated, which for non-normalized expressions caused all partitions to be returned 3. We introduced a race in #3562 that caused operators to sometimes hang on shutdown; this mostly occurred in release builds with high-volume pipelines at the process boundary (no changelog entry for this one as we never released with this bug) 4. The long option `--skip-empty` for `read lines` now works as expected 5. We now include the feature flags in `show build`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
We discussed internally that we want to merge this fix only after the Tenzir v4.3 release, as we want to have some time to double-check whether there are any other fallouts of the change.
Pipelines ending with
serveshould wait until all events are fetched from the corresponding/serveendpoint. This was not always the case because of an incorrectly set scope in the operator.Fixing that scope, however, led to the operator deadlocking the node under some circumstances. This revealed a bug at the very core of CAF's scheduling logic, which incorrectly blocked not just ordinary but also system messages from arriving when a request response was awaited. I got that bug fixed upstream and backported the fix to our bundled fork of CAF 0.18. That in turn revealead us relying on the broken behavior in our execution nodes accidentally. I've included a fix for that as well.