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

avoid rpc waiting too long after connection disconnected #15348

Merged
merged 4 commits into from Apr 7, 2024

Conversation

zhangxu19830126
Copy link
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #15139

What this PR does / why we need it:

Avoid rpc waiting too long after connection disconnected. Currently, when a broken link is detected, all the futures waiting for a response wait until the ctx times out

@matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Apr 7, 2024
@mergify mergify bot added the kind/bug Something isn't working label Apr 7, 2024
@matrix-meow
Copy link
Contributor

@zhangxu19830126 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating the purpose of the changes made.

Body:

The body of the pull request provides relevant information about the type of PR, the specific issue it addresses, and the reason for the changes made. It is structured and informative.

Changes:

  1. In the makeAllWaitingFutureFailed function in backend.go, a new condition has been added to handle cases where a future is waiting for a response but the connection is closed. If the future is still waiting, it is added to a list for further processing. However, if the future is no longer waiting, the messageSent function is called with the backendClosed parameter.

Feedback and Suggestions:

  1. Potential Issue - Incomplete Handling: The addition of f.messageSent(backendClosed) when a future is not waiting may lead to unexpected behavior. It's important to ensure that calling messageSent in this scenario does not have unintended consequences. It would be beneficial to review the logic and potential side effects of this change thoroughly.

  2. Code Optimization - Error Handling: Consider adding error handling or logging mechanisms to handle any issues that may arise when calling messageSent(backendClosed). This will help in identifying and resolving any potential errors or bugs more effectively.

  3. Code Readability - Comments: Adding comments to explain the purpose of the new condition and the messageSent function call can improve code readability and help other developers understand the intention behind the changes more easily.

  4. Testing - Unit Tests: Ensure that appropriate unit tests are added or updated to cover the new condition and the messageSent function call. This will help in verifying the correctness of the changes and prevent regressions in the future.

  5. Security Concern - Connection Closure Handling: Verify that closing the connection and calling messageSent(backendClosed) does not introduce any security vulnerabilities, such as potential resource leaks or unauthorized access. Conduct a security review to address any security implications of this change.

  6. Performance Optimization - Efficiency: Evaluate if there are any opportunities to optimize the code for better performance, especially in scenarios where multiple futures are processed. Consider benchmarking the changes to ensure they do not introduce any performance degradation.

By addressing the potential issues and following the suggestions provided, the quality and reliability of the codebase can be improved, ensuring a more robust and secure implementation.

@matrix-meow matrix-meow added size/S Denotes a PR that changes [10,99] lines and removed size/XS Denotes a PR that changes [1, 9] lines labels Apr 7, 2024
@mergify mergify bot merged commit da7306b into matrixorigin:main Apr 7, 2024
19 checks passed
@zhangxu19830126 zhangxu19830126 mentioned this pull request Apr 8, 2024
7 tasks
zhangxu19830126 added a commit to zhangxu19830126/matrixone that referenced this pull request Apr 10, 2024
This was referenced Apr 10, 2024
sukki37 pushed a commit that referenced this pull request Apr 10, 2024
mergify bot pushed a commit that referenced this pull request Apr 10, 2024
Revert #15348

Approved by: @nnsgmsone
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants