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 wait too long to 1.1 #15373

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

zhangxu19830126
Copy link
Contributor

@zhangxu19830126 zhangxu19830126 commented Apr 8, 2024

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15348

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

@mergify mergify bot requested a review from sukki37 April 8, 2024 02:12
@mergify mergify bot added the kind/bug Something isn't working label Apr 8, 2024
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Apr 8, 2024
@matrix-meow
Copy link
Contributor

@zhangxu19830126 Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is not very clear. It mentions fixing "wait too long to 1.1," which is ambiguous and does not provide a clear indication of the actual changes being made.

Body:

The body of the pull request is informative. It specifies the type of PR (BUG), the related issue (#15348), and the purpose of the changes, which is to avoid RPC waiting too long after a connection is disconnected.

Changes:

  1. In the makeAllWaitingFutureFailed function in backend.go, the changes made involve handling futures waiting for a response after a broken link is detected. The code previously only considered futures that were actively waiting, but now it includes all futures in the process.

    - if f.waiting.Load() {
    + waitings = append(waitings, f)
    + ids = append(ids, id)

    This change removes the condition that checked if a future was actively waiting before adding it to the list of futures to process.

  2. Additionally, the code now distinguishes between futures that are actively waiting and those that are not. If a future is not actively waiting, it calls f.messageSent(backendClosed) instead of f.error(ids[i], backendClosed, nil).

    - f.error(ids[i], backendClosed, nil)
    + if f.waiting.Load() {
    +     f.error(ids[i], backendClosed, nil)
    + } else {
    +     f.messageSent(backendClosed)
    + }

Suggestions for Improvement:

  1. Clarify Title: The title should be more descriptive and directly related to the changes being made. For example, "Improve handling of futures after connection disconnection" could be a more informative title.

  2. Code Optimization:

    • The removal of the condition if f.waiting.Load() and adding all futures to the processing list may introduce unnecessary processing for futures that are not actively waiting. Consider revisiting this logic to ensure only relevant futures are processed.
    • The distinction between actively waiting and non-waiting futures is a good improvement. Ensure that this distinction is necessary for the functionality and does not introduce any unintended side effects.
  3. Error Handling: Verify that the changes made do not impact error handling or error reporting for futures that are not actively waiting. Ensure that all error scenarios are appropriately handled.

  4. Testing: It would be beneficial to include test cases that cover scenarios where futures are actively waiting, not actively waiting, and the behavior after a connection is disconnected to validate the changes made.

  5. Documentation: Update any relevant documentation to reflect the changes made in the code for better understanding and maintenance in the future.

By addressing these suggestions, the code quality, efficiency, and maintainability of the changes can be improved.

@sukki37 sukki37 merged commit 0448606 into matrixorigin:1.1-dev Apr 8, 2024
20 of 23 checks passed
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.

4 participants