-
Notifications
You must be signed in to change notification settings - Fork 125
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
Remove ack-on-drop and ack-on-dead-end. #1670
Conversation
Those behaviours were producing edge cases where error events (from source connectors) were acked, though not handled and thus acked the original failed event. Signed-off-by: Matthias Wahl <matthiaswahl@m7w3.de>
Signed-off-by: Matthias Wahl <matthiaswahl@m7w3.de>
Codecov Report
@@ Coverage Diff @@
## main #1670 +/- ##
==========================================
- Coverage 87.60% 87.58% -0.03%
==========================================
Files 254 254
Lines 50367 50333 -34
==========================================
- Hits 44126 44085 -41
- Misses 6241 6248 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
On a quick glance I didn't see it, does this also un-ack's |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 yes it does I was just blind :D
events: vec![(port, event)], | ||
..EventAndInsights::default() | ||
} | ||
} else if event.transactional { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by removing this we do not ack on drop
in scripts
Those behaviours were producing edge cases where error events (from source connectors) were acked, though not handled and thus acked the original failed event.
Pull request
Description
Related
Checklist
Performance
The performance should be back to where it was before, before we introduced
ack-on-drop
etc.