fix(sdk): opposite retry trigger #2265
Conversation
WalkthroughThe pull request modifies error handling logic in the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/rs-dapi-client/src/transport/grpc.rs (1)
Line range hint
125-136: Approve changes with a suggestion for improved readabilityThe modification to the
can_retrymethod correctly implements the retry logic for gRPC status codes. This change aligns with the PR title "fix(sdk): opposite retry trigger" and should resolve any issues related to incorrect retry behavior.To improve readability, consider grouping related status codes and adding a brief comment explaining the rationale for each group. For example:
matches!( code, // Transient errors that may resolve on retry Ok | Cancelled | Unknown | DeadlineExceeded | ResourceExhausted | // Server-side issues that might be temporary Aborted | Internal | Unavailable | // Data integrity issues that might be resolved by retrying DataLoss )This grouping and commenting can make the code more maintainable and easier to understand for future developers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/rs-dapi-client/src/dapi_client.rs (1 hunks)
- packages/rs-dapi-client/src/transport/grpc.rs (1 hunks)
🧰 Additional context used
🔇 Additional comments (1)
packages/rs-dapi-client/src/dapi_client.rs (1)
Line range hint
236-248: Clarify the intention behind inverting the retry conditionThe error handling logic has been significantly altered:
- Previously, addresses were banned for non-retryable errors.
- Now, addresses are banned for retryable errors.
This change raises some concerns:
- Banning addresses for retryable errors might unnecessarily reduce the available address pool.
- Non-retryable errors no longer trigger address banning, potentially keeping problematic addresses in the pool.
While this change aligns with the PR title mentioning "opposite retry trigger", it's unclear if this is the intended behavior. Could you please clarify:
- Is this inversion intentional?
- What's the rationale behind banning addresses for retryable errors?
- How do we handle persistently problematic addresses if we don't ban them for non-retryable errors?
To help verify the impact of this change, please run the following script:
This will help us understand if this change is consistent with error handling in other parts of the codebase.
DAPI Client is supposed to retry in case of errors that originated by a node but not by a user. For example, we should retry if the node is unavailable but not retry if the user broadcasted a state transition with an invalid structure.
Issue being fixed or feature implemented
The logic described above was reverted mistakenly.
What was done?
can_retrycalls to retry in case of node issues but not user.How Has This Been Tested?
None
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
New Features
Bug Fixes
Documentation