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 can not connect to remote backend when down cn #16329

Merged
merged 2 commits into from
May 23, 2024

Conversation

iamlinjunhong
Copy link
Contributor

@iamlinjunhong iamlinjunhong commented May 22, 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 #https://github.com/matrixorigin/MO-Cloud/issues/3288 #15996

What this PR does / why we need it:

fix can not connect to remote backend when down cn

retry lock for CN rolling restart is too excessive

@mergify mergify bot requested a review from sukki37 May 22, 2024 11:01
@matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 22, 2024
@matrix-meow
Copy link
Contributor

@iamlinjunhong Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that it fixes an issue related to connecting to a remote backend when a specific condition is met.

Body:

The body of the pull request includes information about the type of PR, the related issue, and a brief description of what the PR does. It would be beneficial to provide more context or details about the fix in the body to help reviewers understand the changes better.

Changes:

  1. In the service_test.go file:

    • A new test function TestIssue3288 has been added to test the behavior when a lock service is closed and an attempt to lock is made on another service.
    • The test checks if the lock fails due to the backend being unable to connect and then succeeds after a delay.
    • The test seems to cover the scenario described in the issue contributors readme action update #3288.
  2. In the lock_op.go file:

    • Changes have been made to the canRetryLock function to include additional error conditions related to backend closure and connection failure.

Suggestions for Improvement:

  1. Body Description: Enhance the body of the pull request with more detailed information about the issue and the specific changes made to address it. This will provide better context for reviewers and future reference.

  2. Error Handling: In the canRetryLock function, the condition !moerr.IsMoErrCode(err, moerr.ErrBackendClosed) seems redundant as it is already covered by the !moerr.IsMoErrCode(err, moerr.ErrLockTableBindChanged) condition. It may be unnecessary to include it unless there is a specific reason for checking it separately.

  3. Optimization:

    • Consider refactoring the test function TestIssue3288 to improve readability and maintainability. Break down the test logic into smaller, more focused functions if possible.
    • Ensure that the test cases cover edge cases and potential failure scenarios comprehensively to validate the fix thoroughly.
  4. Security Concern:

    • While the changes seem focused on functionality, it's essential to ensure that handling of errors related to backend closure and connection failure is done securely. Verify that sensitive information or resources are not exposed due to these error conditions.

By addressing the suggestions and optimizing the changes, the pull request can be improved in terms of clarity, efficiency, and security.

@mergify mergify bot merged commit c69d33d into matrixorigin:1.2-dev May 23, 2024
19 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes [10,99] lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants