-
Notifications
You must be signed in to change notification settings - Fork 21
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
feat: allow DataStreamBuilder to retry queries if non fatal err #1711
feat: allow DataStreamBuilder to retry queries if non fatal err #1711
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1711 +/- ##
===========================================
- Coverage 40.10% 18.59% -21.52%
===========================================
Files 26 100 +74
Lines 1895 12270 +10375
Branches 1895 12270 +10375
===========================================
+ Hits 760 2281 +1521
- Misses 1100 9557 +8457
- Partials 35 432 +397 ☔ View full report in Codecov by Sentry. |
57cc457
to
7a23b58
Compare
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.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @noamsp-starkware)
crates/papyrus_p2p_sync/src/client/stream_builder.rs
line 119 at r1 (raw file):
Err(ParseDataError::BadPeer(err)) => { warn!( "Query for {:?} returned with bad peer error: {:?}. retrying query.",
retrying query -> reporting peer and retrying query
crates/papyrus_p2p_sync/src/client/stream_builder.rs
line 119 at r1 (raw file):
Err(ParseDataError::BadPeer(err)) => { warn!( "Query for {:?} returned with bad peer error: {:?}. retrying query.",
I think it's worth logging the block number. The same is true for the log above this one
"Query for {:?} on {:?} returned ...", Self::TYPE_DESCRIPTION, current_block_number, err)
Create err types BadPeerError,ParseDataError. later on the errors from P2PSyncClientError will be sorted to fatal and non-fatal(BadPeer). this will allow to retry on non fatal errors during parsing of data blocks.
moves non-fatal state diff related errors to BadPeerError to allow retrying the query with a different peer. previously state diff related tests checked these non-fatal errors as assertions to certain scenarios. we now just check that these scenarios cause peer reporting.
da56660
to
780199e
Compare
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.
Reviewed 3 of 3 files at r2, 4 of 4 files at r3, 4 of 4 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware)
Create err types BadPeerError,ParseDataError. later on the errors from P2PSyncClientError will be sorted to fatal and non-fatal(BadPeer). this will allow to retry on non fatal errors during parsing of data blocks.