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 deug log #16820

Merged
merged 2 commits into from
Jun 12, 2024
Merged

add deug log #16820

merged 2 commits into from
Jun 12, 2024

Conversation

jiangxinmeng1
Copy link
Contributor

@jiangxinmeng1 jiangxinmeng1 commented Jun 12, 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 #16812

What this PR does / why we need it:

add debug log


PR Type

Enhancement, Bug fix


Description

  • Added methods CheckPrintPrepareCompact and PrintPrepareCompactDebugLog to ObjectEntry for logging debug information related to compact operations.
  • Enhanced PrepareCompact methods in aobject and object to include debug logging.
  • Introduced a boolean flag hasPrintedPrepareComapct to track if the debug log has already been printed.

Changes walkthrough 📝

Relevant files
Enhancement
object.go
Add debug logging for compact operations in ObjectEntry   

pkg/vm/engine/tae/catalog/object.go

  • Added CheckPrintPrepareCompact method to check if a compact operation
    should be logged.
  • Added PrintPrepareCompactDebugLog method to log debug information for
    compact operations.
  • Introduced hasPrintedPrepareComapct boolean to track if the debug log
    has been printed.
  • +45/-0   
    aobj.go
    Enhance PrepareCompact with debug logging in aobject         

    pkg/vm/engine/tae/tables/aobj.go

  • Enhanced PrepareCompact method to include debug logging for compact
    operations.
  • Added calls to CheckPrintPrepareCompact and
    PrintPrepareCompactDebugLog methods.
  • +11/-1   
    obj.go
    Enhance PrepareCompact with debug logging in object           

    pkg/vm/engine/tae/tables/obj.go

  • Enhanced PrepareCompact method to include debug logging for compact
    operations.
  • Added calls to CheckPrintPrepareCompact and
    PrintPrepareCompactDebugLog methods.
  • +5/-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 Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    3

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Possible Bug:
    The variable name hasPrintedPrepareComapct appears to be a typo. It should likely be hasPrintedPrepareCompact.

    Performance Concern:
    The debug logging logic in PrintPrepareCompactDebugLog method could potentially slow down the system if the logging is extensive due to the loop over object iterations and multiple lock operations.

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 12, 2024
    @mergify mergify bot requested a review from sukki37 June 12, 2024 05:41
    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
    Maintainability
    Correct a typo in the variable name to maintain consistency

    The variable name hasPrintedPrepareComapct contains a typo. It should be
    hasPrintedPrepareCompact to maintain consistency and avoid confusion.

    pkg/vm/engine/tae/catalog/object.go [45]

    -hasPrintedPrepareComapct bool
    +hasPrintedPrepareCompact bool
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies and fixes a typo in the variable name, which is crucial for code clarity and maintainability.

    10
    Possible bug
    Add a nil check for lastNode to prevent potential runtime panics

    Add a check to ensure lastNode is not nil before accessing its methods to prevent
    potential runtime panics.

    pkg/vm/engine/tae/catalog/object.go [657-658]

     lastNode := entry.GetLatestNodeLocked()
    +if lastNode == nil {
    +    return false
    +}
     startTS := lastNode.GetStart()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: This suggestion is essential as it addresses a potential runtime panic by adding a nil check before accessing methods on lastNode.

    10
    Enhancement
    Simplify the function by removing redundant variables and directly returning the result

    Simplify the PrepareCompact function by directly returning the result of
    obj.meta.PrepareCompact() and removing the redundant variable.

    pkg/vm/engine/tae/tables/obj.go [66-70]

    -prepareCompact := obj.meta.PrepareCompact()
    -if !prepareCompact && obj.meta.CheckPrintPrepareCompact() {
    +if !obj.meta.PrepareCompact() && obj.meta.CheckPrintPrepareCompact() {
         obj.meta.PrintPrepareCompactDebugLog()
    +    return false
     }
    -return prepareCompact
    +return true
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: This suggestion simplifies the function significantly, enhancing readability and reducing unnecessary code, which is beneficial for maintenance.

    8
    Combine repeated conditions to reduce redundancy and improve readability

    Combine the repeated CheckPrintPrepareCompact condition to reduce redundancy and improve
    readability.

    pkg/vm/engine/tae/tables/aobj.go [126-143]

     if obj.meta.CheckPrintPrepareCompact() {
         obj.meta.PrintPrepareCompactDebugLog()
    -}
    -...
    -if obj.meta.CheckPrintPrepareCompact() {
         logutil.Infof("obj %v, data prepare compact failed", obj.meta.ID.String())
    -}
    -...
    -if !prepareCompact && obj.meta.CheckPrintPrepareCompact() {
    -    logutil.Infof("obj %v, data ref count > 0", obj.meta.ID.String())
    +    if !prepareCompact {
    +        logutil.Infof("obj %v, data ref count > 0", obj.meta.ID.String())
    +    }
     }
     
    Suggestion importance[1-10]: 7

    Why: The suggestion improves code readability and efficiency by combining repeated conditions, although it's not a critical issue.

    7

    @mergify mergify bot merged commit d560d11 into matrixorigin:1.2-dev Jun 12, 2024
    16 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix Enhancement Review effort [1-5]: 3 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