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

[bug] logtail: clear the tables in global stats when logtail re-connect #18674

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

volgariver6
Copy link
Contributor

@volgariver6 volgariver6 commented Sep 10, 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 https://github.com/matrixorigin/MO-Cloud/issues/4045

What this PR does / why we need it:

clear the tables in global stats when logtail re-connect.


PR Type

Bug fix, Tests


Description

  • Added a clearTables method in GlobalStats to clear the tables in the map.
  • Integrated the clearTables method into the engine initialization process to ensure tables are cleared in global stats when logtail reconnects.
  • Added a unit test to verify the functionality of the clearTables method.

Changes walkthrough 📝

Relevant files
Bug fix
db.go
Clear tables in global stats during engine initialization

pkg/vm/engine/disttae/db.go

  • Added a call to clear all tables in global stats during
    initialization.
  • +3/-0     
    Enhancement
    stats.go
    Implement clearTables method in GlobalStats                           

    pkg/vm/engine/disttae/stats.go

  • Implemented clearTables method to clear tables in the global stats
    map.
  • +9/-0     
    Tests
    stats_test.go
    Add test for clearTables method in GlobalStats                     

    pkg/vm/engine/disttae/stats_test.go

    • Added a test for the clearTables method in GlobalStats.
    +13/-0   

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Potential Race Condition
    The clearTables method uses a lock, but the RemoveTid method doesn't. This could lead to a race condition if these methods are called concurrently.

    @mergify mergify bot requested a review from sukki37 September 10, 2024 03:47
    @mergify mergify bot added the kind/bug Something isn't working label Sep 10, 2024
    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Sep 10, 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
    Enhancement
    Simplify the code by removing an unnecessary condition

    Consider removing the condition if len(gs.logtailUpdate.mu.updated) > 0 as it's not
    necessary. Assigning a new empty map is efficient regardless of the current map's
    size.

    pkg/vm/engine/disttae/stats.go [273-275]

    -if len(gs.logtailUpdate.mu.updated) > 0 {
    -  gs.logtailUpdate.mu.updated = make(map[uint64]struct{})
    -}
    +gs.logtailUpdate.mu.updated = make(map[uint64]struct{})
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Removing the condition if len(gs.logtailUpdate.mu.updated) > 0 simplifies the code without affecting functionality, as assigning a new empty map is efficient regardless of the current map's size.

    8
    Maintainability
    Rename the method to better reflect its specific functionality

    Consider using a more descriptive name for the clearTables method, such as
    clearUpdatedTables or resetLogtailUpdates, to better reflect its specific
    functionality.

    pkg/vm/engine/disttae/stats.go [269-276]

    -// clearTables clears the tables in the map if there are any tables in it.
    -func (gs *GlobalStats) clearTables() {
    +// clearUpdatedTables clears the updated tables in the map if there are any.
    +func (gs *GlobalStats) clearUpdatedTables() {
       gs.logtailUpdate.mu.Lock()
       defer gs.logtailUpdate.mu.Unlock()
       if len(gs.logtailUpdate.mu.updated) > 0 {
         gs.logtailUpdate.mu.updated = make(map[uint64]struct{})
       }
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to rename clearTables to clearUpdatedTables or resetLogtailUpdates improves code readability and maintainability by providing a more descriptive method name that reflects its functionality.

    7

    @mergify mergify bot merged commit eebb872 into matrixorigin:1.2-dev Sep 10, 2024
    17 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 kind/bug Something isn't working Review effort [1-5]: 2 size/S Denotes a PR that changes [10,99] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants