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

[cherry-pick-16424] : add log to track 16273 #16472

Merged
merged 3 commits into from
May 29, 2024

Conversation

jensenojs
Copy link
Contributor

@jensenojs jensenojs commented May 29, 2024

User description

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #16273

What this PR does / why we need it:

add log to track 16273


PR Type

Bug fix


Description

  • Added detailed error logging in the Delete function of engine.go to track issues related to database deletion failures.
  • Included stack trace and transaction metadata in the logs when the database name contains "sysbench_db".

Changes walkthrough 📝

Relevant files
Bug fix
engine.go
Add detailed error logging in `Delete` function                   

pkg/vm/engine/disttae/engine.go

  • Added import for stack package.
  • Modified Delete function to include error logging.
  • Added detailed error logs for Delete function when the database name
    contains "sysbench_db".
  • +12/-1   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves a moderate amount of changes with the addition of error handling and logging in a specific function. The logic is straightforward, focusing on enhanced error reporting, which makes it relatively easy to review.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Error Handling Scope: The error handling and logging are scoped only to database names containing "sysbench_db". This might not catch errors in other databases which could also benefit from detailed error logs.

    Logging Verbosity: The addition of stack trace logging could potentially lead to verbose logs, especially in production environments, which might affect log readability and management.

    🔒 Security concerns

    No

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label May 29, 2024
    @mergify mergify bot requested a review from sukki37 May 29, 2024 06:37
    @mergify mergify bot added the kind/bug Something isn't working label May 29, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    Add a nil check for op.Txn() to prevent potential nil pointer dereference

    Ensure that the op.Txn() call is safe and cannot result in a nil pointer dereference by
    adding a nil check before calling DebugString().

    pkg/vm/engine/disttae/engine.go [541]

    -logutil.Errorf("txnmeta %v", op.Txn().DebugString())
    +if txn := op.Txn(); txn != nil {
    +    logutil.Errorf("txnmeta %v", txn.DebugString())
    +} else {
    +    logutil.Errorf("txnmeta is nil")
    +}
     
    Suggestion importance[1-10]: 8

    Why: Adding a nil check before accessing methods on potentially nil objects is crucial to prevent runtime errors such as nil pointer dereferences, making this a valuable suggestion.

    8
    Maintainability
    Extract the logging logic into a separate function to improve readability and maintainability

    To improve readability and maintainability, consider extracting the logging logic into a
    separate function. This will make the Delete function cleaner and easier to understand.

    pkg/vm/engine/disttae/engine.go [538-542]

    -if strings.Contains(name, "sysbench_db") {
    -    logutil.Errorf("delete database %s failed: %v", name, err)
    -    logutil.Errorf("stack: %s", stack.Callers(3))
    -    logutil.Errorf("txnmeta %v", op.Txn().DebugString())
    +logDeleteError(name, err, op)
    +
    +func logDeleteError(name string, err error, op client.TxnOperator) {
    +    if strings.Contains(name, "sysbench_db") {
    +        logutil.Errorf("delete database %s failed: %v", name, err)
    +        logutil.Errorf("stack: %s", stack.Callers(3))
    +        logutil.Errorf("txnmeta %v", op.Txn().DebugString())
    +    }
     }
     
    Suggestion importance[1-10]: 7

    Why: Extracting the logging logic into a separate function is a good practice for maintainability and readability, especially when the logging is non-trivial as in this case.

    7
    Possible issue
    Use a more specific condition to check the database name to avoid false positives

    Instead of using strings.Contains to check if the name contains "sysbench_db", consider
    using a more specific condition if possible. This will avoid potential false positives
    where "sysbench_db" might be a substring of another name.

    pkg/vm/engine/disttae/engine.go [538]

    -if strings.Contains(name, "sysbench_db") {
    +if name == "sysbench_db" {
     
    Suggestion importance[1-10]: 5

    Why: The suggestion to use a more specific condition is valid to avoid false positives, but changing to an exact match might not cover all intended cases if 'sysbench_db' is a prefix or category rather than a specific identifier.

    5
    Enhancement
    Reduce the stack trace depth to capture a more relevant part of the stack trace

    Consider reducing the stack trace depth from 3 to 2 in stack.Callers(3) to capture a more
    relevant part of the stack trace, as the current depth might include unnecessary frames.

    pkg/vm/engine/disttae/engine.go [540]

    -logutil.Errorf("stack: %s", stack.Callers(3))
    +logutil.Errorf("stack: %s", stack.Callers(2))
     
    Suggestion importance[1-10]: 3

    Why: The suggestion to adjust the stack trace depth is minor and its impact is uncertain without specific knowledge of the call stack's relevance at different depths.

    3

    @mergify mergify bot merged commit bc1c1b1 into matrixorigin:1.2-dev May 29, 2024
    17 of 18 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
    Bug fix kind/bug Something isn't working Review effort [1-5]: 2 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