-
Notifications
You must be signed in to change notification settings - Fork 6
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 recovery in datasets #527
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
- Coverage 82.49% 82.37% -0.13%
==========================================
Files 215 215
Lines 10054 10080 +26
==========================================
+ Hits 8294 8303 +9
- Misses 1760 1777 +17 ☔ View full report in Codecov by Sentry. |
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.
my question is on the raise e
in all four changed methods. It makes sense in this case to make a unit test to ensure everything works under any case (error-free case, case with error)
Can you piggyback deleting this log
len(s.triggers) is not the number of triggers we this time are processing.
|
Before, we had recovery logic based on reply indices. However, while working on storage, I realized those responses come non deterministically from multiple threads. Hence, we cannot rely on the ordering. We need to keep track of the sample IDs we already yielded. I changed the logic to just keep a list which is cheap to append to, and only convert to a set / hash table as soon as we failed once and we actually need to do many
in
checks.