-
Notifications
You must be signed in to change notification settings - Fork 143
fix: replace panic unwrap with error check in cluster.rs (#504) #508
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
Conversation
|
Welcome @SaathwikDasari! |
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.
Pull request overview
This PR addresses a critical panic issue in the TiKV client library where calling .unwrap() on resp.leader caused application crashes when the leader was None. The fix replaces the panic-inducing unwrap with proper error handling using ok_or_else.
Key changes:
- Replaced
.unwrap()with.ok_or_else()to return a proper error instead of panicking - Added descriptive error message "no leader found in GetMembersResponse"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
The But I think it's still nice to have this change to make it more robust. @SaathwikDasari Please fix the error of "CI / check". Rest LGTM. |
Signed-off-by: Saathwik Dasari <saathwik.dasari@gmail.com>
Signed-off-by: Saathwik Dasari <saathwik.dasari@gmail.com>
f6205d8 to
85ebfa7
Compare
|
cc @iosmanthus |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iosmanthus, pingyu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
@SaathwikDasari Thanks for your contribution ! |
What is changed and how it works?
Replaced a panic-inducing
.unwrap()onresp.leaderwithok_or_elseinsrc/pd/cluster.rs.If
resp.leaderisNone, the client will now return a proper error instead of crashing the entire application.Issue reference
Closes #504
Check List
cargo fmt? If not, run it locally now!)