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 panic #16776

Merged
merged 5 commits into from
Jun 11, 2024
Merged

fix panic #16776

merged 5 commits into from
Jun 11, 2024

Conversation

daviszhen
Copy link
Contributor

@daviszhen daviszhen commented Jun 11, 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 ##16724

#16752

What this PR does / why we need it:

修改在mo server还未启动时metric就开始执行的时序问题。


PR Type

Bug fix, Enhancement


Description

  • Added functionality to ensure metrics and storage usage calculations wait for the MO server to start.
  • Introduced thread safety in NullResp methods by adding mutex locking.
  • Reordered imports in cmd/mo-service/main.go for consistency.

Changes walkthrough 📝

Relevant files
Enhancement
main.go
Ensure metrics initialization waits for server start         

cmd/mo-service/main.go

  • Added mometric.WithFrontendServerStarted to InitMetric call.
  • Reordered imports for consistency.
  • +6/-3     
    server.go
    Track and check MO server start state                                       

    pkg/frontend/server.go

  • Added moServerStarted atomic boolean to track server start state.
  • Updated Start method to set moServerStarted to true upon successful
    start.
  • Added MoServerIsStarted and setMoServerStarted functions.
  • +17/-2   
    cron_task.go
    Ensure storage usage calculation waits for server start   

    pkg/util/metric/mometric/cron_task.go

  • Added frontendServerStarted function to check server start state.
  • Updated CalculateStorageUsage to wait for server start.
  • +16/-8   
    metric.go
    Add frontend server start check to metric options               

    pkg/util/metric/mometric/metric.go

  • Added frontendServerStarted to InitOptions.
  • Added WithFrontendServerStarted option to set frontendServerStarted.
  • +9/-0     
    Bug fix
    resp_client.go
    Add thread safety to `NullResp` methods                                   

    pkg/frontend/resp_client.go

    • Added mutex locking to NullResp methods for thread safety.
    +12/-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 [1-5]

    3

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    Thread Safety:
    The use of mutex in NullResp methods to ensure thread safety is a good practice. However, it's important to ensure that all methods that modify or access the shared state are properly synchronized.

    Metric Initialization Timing:
    The modification to delay metric initialization until the server starts is crucial to avoid race conditions. Reviewers should ensure that the implementation correctly handles all edge cases where the server might not start as expected.

    Error Handling:
    In the Start method of MOServer, the error handling after attempting to start the application should be reviewed to ensure that it properly handles and logs errors.

    @mergify mergify bot requested a review from sukki37 June 11, 2024 03:30
    @mergify mergify bot added the kind/bug Something isn't working label Jun 11, 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 issue
    Use a defer statement to ensure the cancel function is called to avoid potential memory leaks

    To avoid potential memory leaks, ensure that the cancel function is called in the
    CalculateStorageUsage function by using a defer statement.

    pkg/util/metric/mometric/cron_task.go [124]

     ctx, cancel := context.WithCancel(ctx)
    +defer cancel()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential resource leak by ensuring that the cancel function is always called, which is critical for resource management and avoiding memory leaks in long-running processes.

    9
    Best practice
    Use a defer statement to ensure the server started status is set even if an error occurs

    To ensure that setMoServerStarted is always called even if mo.app.Start() returns an
    error, consider using a defer statement to set the server started status.

    pkg/frontend/server.go [70-74]

     err := mo.app.Start()
    +defer setMoServerStarted(true)
     if err != nil {
         return err
     }
    -setMoServerStarted(true)
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a defer statement for setMoServerStarted(true) ensures that the server's started status is reliably set, which is crucial for the correct functioning of the server, especially in error scenarios.

    8
    Performance
    Use sync.RWMutex instead of sync.Mutex for better performance with more reads than writes

    Instead of using sync.Mutex directly in the NullResp struct, consider using sync.RWMutex
    to allow multiple readers and a single writer, which can improve performance when there
    are more reads than writes.

    pkg/frontend/resp_client.go [166]

    -sync.Mutex
    +sync.RWMutex
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: This is a valid suggestion for performance improvement, especially if the NullResp struct is read frequently compared to how often it is written to.

    7
    Readability
    Break down the InitMetric call into multiple lines for better readability

    To improve readability and maintainability, consider breaking the InitMetric call into
    multiple lines with each parameter on a new line. This will make it easier to see all the
    parameters being passed.

    cmd/mo-service/main.go [490-493]

    -if act := mometric.InitMetric(ctx, nil, &SV, UUID, nodeRole,
    +if act := mometric.InitMetric(
    +    ctx, nil, &SV, UUID, nodeRole,
         mometric.WithWriterFactory(writerFactory),
         mometric.WithFrontendServerStarted(frontend.MoServerIsStarted),
     ); !act {
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion correctly identifies a readability improvement in the InitMetric call by breaking it into multiple lines, making the parameters more visible.

    6

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 11, 2024
    @mergify mergify bot merged commit d44af1b into matrixorigin:1.2-dev Jun 11, 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 Enhancement kind/bug Something isn't working 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.

    7 participants