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

add error log in taskservice #16018

Merged

Conversation

w-zr
Copy link
Contributor

@w-zr w-zr commented May 11, 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 #16010

What this PR does / why we need it:

Add error log in taskservice when task service cannot get db on 1.2-dev.

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

@w-zr Thanks for your contributions!

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating the specific improvement being made in the codebase.

Body:

The body of the pull request provides relevant information about the type of PR, the specific issue it fixes, and the reason for the changes. It also mentions adding error logs in the task service when the service cannot get the database on a specific branch.

Changes:

  1. In mysql_task_storage.go:

    • The change made in the useDB function now uses errors.Join to combine the original error with ErrNotReady before returning.
    • Issue: The usage of errors.Join is not a standard Go function for combining errors. The correct function to use for combining errors is fmt.Errorf.
    • Suggestion: Replace errors.Join(err, ErrNotReady) with fmt.Errorf("error message: %v, %w", err, ErrNotReady).
  2. In task_service.go:

    • In the CreateAsyncTask and CreateBatch functions, the error handling logic has been updated to check if the error is ErrNotReady using errors.Is and then retrying after a delay.
    • Issue: The error handling logic could be improved by providing more context in the error messages.
    • Suggestion: Add more descriptive error messages to the logs to help with debugging and troubleshooting.

Security Concern:

  • The use of errors.Join in the mysql_task_storage.go file can potentially expose sensitive error information to users if not handled properly.
  • Suggestion: Ensure that error messages logged do not contain sensitive information and consider logging only non-sensitive error details.

Optimization:

  • Consider refactoring the error handling logic in the CreateAsyncTask and CreateBatch functions to make it more modular and reusable across the codebase.
  • Suggestion: Extract the common error handling logic into a separate function to avoid code duplication and improve maintainability.

By addressing the mentioned issues and suggestions, the code quality and maintainability of the project can be enhanced. Additionally, ensuring proper error handling practices will contribute to a more robust and secure application.

@mergify mergify bot merged commit bb20ff7 into matrixorigin:1.2-dev May 11, 2024
18 of 19 checks passed
@aylei aylei mentioned this pull request Jun 5, 2024
@w-zr w-zr deleted the add-error-info-in-taskservice-1.2-dev branch July 4, 2024 12:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement 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