-
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
base: master
Are you sure you want to change the base?
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
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.
src/pd/cluster.rs
Outdated
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; | ||
|
|
Copilot
AI
Jan 2, 2026
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.
There is trailing whitespace at the end of this line. Please remove it to maintain code cleanliness.
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; | |
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; |
src/pd/cluster.rs
Outdated
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; | ||
|
|
Copilot
AI
Jan 2, 2026
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.
This empty line appears to contain trailing whitespace and should be removed. The blank line between the error handling and the for loop is not necessary and adds unnecessary whitespace.
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; | |
| .ok_or_else(|| internal_err!("no leader found in GetMembersResponse"))?; |
|
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
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!)