Skip to content
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: refresh leader on disconnect #1140

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

shihuili1218
Copy link
Collaborator

@shihuili1218 shihuili1218 commented Aug 12, 2024

Motivation:

Explain the context, and why you're making that change.
To make others understand what is the problem you're trying to solve.

Modification:

Describe the idea and modifications you've done.

Result:

Fixes #.

If there is no issue then describe the changes introduced by this PR.

Summary by CodeRabbit

  • New Features

    • Introduced ConnectionFailureException for improved error handling related to RPC connection failures.
    • Added a utility method getRootCause to enhance exception diagnostics.
  • Bug Fixes

    • Enhanced error handling in BoltRpcClient to differentiate connection failures from other remoting exceptions.
    • Streamlined error management in DefaultRheaKVRpcService and DefaultPlacementDriverRpcService for consistency in handling connection errors.
  • Documentation

    • Updated error handling logic to improve clarity and maintainability across various services.

@sofastack-cla sofastack-cla bot added bug Something isn't working cla:yes size/L labels Aug 12, 2024
Copy link
Contributor

coderabbitai bot commented Aug 12, 2024

Walkthrough

Recent changes enhance the error handling mechanisms in the jraft framework by introducing a new ConnectionFailureException for RPC connection issues. Modifications across several classes improve the granularity of error reporting, allowing for better diagnostics. The framework now distinguishes connection failures from general remoting exceptions, streamlining error management and improving the clarity of the codebase.

Changes

File Change Summary
.../error/ConnectionFailureException.java Added ConnectionFailureException class extending RemotingException for RPC connection errors.
.../rpc/impl/BoltRpcClient.java Updated error handling in invokeSync and invokeAsync to throw ConnectionFailureException.
.../util/internal/ThrowUtil.java Introduced getRootCause method; modified cutCause method to use root causes for better reporting.
.../client/DefaultRheaKVRpcService.java Simplified exception handling to use ConnectionFailureException instead of RemotingException.
.../client/pd/DefaultPlacementDriverRpcService.java Replaced RemotingException handling with ConnectionFailureException in complete and executor methods.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant BoltRpcClient
    participant ConnectionFailureException
    participant RheaKVRpcService

    Client->>BoltRpcClient: invokeSync(request)
    BoltRpcClient-->>ConnectionFailureException: Check for ConnectException
    ConnectionFailureException-->>BoltRpcClient: Throw ConnectionFailureException
    BoltRpcClient->>RheaKVRpcService: Handle connection error
Loading

🐰 In the land of code where rabbits play,
New exceptions bloom to brighten the day.
Connections that falter, now caught in their plight,
With clarity shining, the errors take flight.
Hop, skip, and jump, through logic so bright,
The jraft framework leaps to a new height! 🌼


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?

Share
Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai generate interesting stats about this repository and render them as a table.
    • @coderabbitai show all the console.log statements in this repository.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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 as PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between ea1e925 and 32760ff.

Files selected for processing (5)
  • jraft-core/src/main/java/com/alipay/sofa/jraft/error/ConnectionFailureException.java (1 hunks)
  • jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/BoltRpcClient.java (4 hunks)
  • jraft-core/src/main/java/com/alipay/sofa/jraft/util/internal/ThrowUtil.java (1 hunks)
  • jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/DefaultRheaKVRpcService.java (3 hunks)
  • jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/pd/DefaultPlacementDriverRpcService.java (3 hunks)
Additional comments not posted (9)
jraft-core/src/main/java/com/alipay/sofa/jraft/error/ConnectionFailureException.java (1)

1-45: New Exception Class Added: ConnectionFailureException.

The new class ConnectionFailureException is well-structured, providing multiple constructors for flexibility in handling RPC connection failures. This addition enhances error handling granularity.

jraft-core/src/main/java/com/alipay/sofa/jraft/util/internal/ThrowUtil.java (2)

69-75: New Method Added: getRootCause.

The getRootCause method effectively traverses the cause chain to find the root cause of a Throwable. This utility enhances error diagnostics.


64-64: Improvement in cutCause Method.

The cutCause method now sets the stack trace to that of the root cause, enhancing the accuracy of error reporting.

jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/pd/DefaultPlacementDriverRpcService.java (3)

28-28: Improved Error Handling: ConnectionFailureException Import.

The import of ConnectionFailureException aligns with the enhanced error handling strategy focused on connection failures.


114-114: Simplified Error Handling in complete Method.

The removal of specific handling for RemotingException in favor of a generalized approach simplifies the error handling logic.


126-132: Refined Error Handling in executor Method.

The catch block now explicitly handles ConnectionFailureException, improving specificity and robustness in error management.

jraft-core/src/main/java/com/alipay/sofa/jraft/rpc/impl/BoltRpcClient.java (2)

124-126: Consistent error handling is approved.

The handling of ConnectException in invokeAsync aligns with invokeSync, ensuring consistency.

Verify that ConnectionFailureException is consistently handled throughout the codebase.


106-108: Enhanced error handling is approved.

The addition of ConnectionFailureException for ConnectException improves error granularity and diagnostics.

Ensure that ConnectionFailureException is properly handled in other parts of the codebase to maintain robustness.

Verification successful

ConnectionFailureException is properly handled in the codebase.

The exception is checked in key classes such as DefaultRheaKVRpcService and DefaultPlacementDriverRpcService, indicating that the error handling is robust and consistent across the codebase.

  • DefaultRheaKVRpcService: Checks for ConnectionFailureException.
  • DefaultPlacementDriverRpcService: Checks for ConnectionFailureException.
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify handling of `ConnectionFailureException` throughout the codebase.

# Test: Search for the usage of `ConnectionFailureException`. Expect: Proper handling in all relevant locations.
rg --type java 'ConnectionFailureException'

Length of output: 2976

jraft-rheakv/rheakv-core/src/main/java/com/alipay/sofa/jraft/rhea/client/DefaultRheaKVRpcService.java (1)

156-161: Streamlined error handling is approved.

The specific handling of ConnectionFailureException improves clarity and reduces redundancy.

Ensure that ConnectionFailureException handling is consistent across the codebase.

Copy link
Contributor

@fengjiachun fengjiachun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you! @shihuili1218

@fengjiachun fengjiachun merged commit 8e1de66 into sofastack:master Aug 12, 2024
9 checks passed
@fengjiachun
Copy link
Contributor

This change won't affect the Jepsen test results. I think we can release 1.3.15.bugfix immediately. @shihuili1218

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cla:yes size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants