-
Notifications
You must be signed in to change notification settings - Fork 14
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
fix: duplicate message - WPB-11435 #2657
base: develop
Are you sure you want to change the base?
Conversation
…-16359 (#2631) Co-authored-by: François Benaiteau <[email protected]>
Co-authored-by: Jullian Mercier <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Christoph Aldrian <[email protected]> Co-authored-by: Christoph Aldrian <[email protected]> Co-authored-by: Sam Wyndham <[email protected]>
Test Results 1 files 1 suites 1m 34s ⏱️ For more details on these failures, see this check. Results for commit afaa539. ♻️ This comment has been updated with latest results. |
Datadog ReportBranch report: ✅ 0 Failed, 1108 Passed, 0 Skipped, 1m 34.68s Total Time |
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.
LGTM, just one non blocking comment about events batch processing logic
) async { | ||
guard !isProcessing else { | ||
WireLogger.updateEvent.debug("⚠️ process called before end of processing, abort") | ||
return |
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.
question(non-blocking): so if a second events batch wants to be processed while the first one is not finished yet, we discard it (early return). And I guess it will be processed again once the first is finished otherwise we'll lose that events batch, right ?
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.
Or maybe this logic only concerns the duplicated events since the function is recursive. Just checking that we don't lose any events along the way.
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.
to be honest I am not sure, this might be not completely correct
… and before setting the epoch - WPB-16001 (#2647)
Issue
On the legacy code, it happens message get processed multiple times.
It could be that in the following recursive method, because of a suspension point, the
fetchNextEventsBatch
fetch the same event before we actually delete it from the database and so reprocess it.isProcessing
so we don't process twice the same event.Testing
Actual:
You'll see duplicate messages
Fix:
You'll get no duplicate messages
Checklist
[WPB-XXX]
.UI accessibility checklist
If your PR includes UI changes, please utilize this checklist: