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

remove log print from automaxprocs to 1.2-dev #16599

Merged
merged 3 commits into from
Jun 3, 2024

Conversation

badboynt1
Copy link
Contributor

@badboynt1 badboynt1 commented Jun 3, 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/3374

What this PR does / why we need it:

remove log print from automaxprocs to 1.2-dev


PR Type

Bug fix


Description

  • Added initialization for go.uber.org/automaxprocs/maxprocs logger to suppress log output in cmd/mo-service/main.go.
  • Removed unused import for go.uber.org/automaxprocs in pkg/sql/compile/compile.go.
  • Removed unused import for go.uber.org/automaxprocs in pkg/sql/compile/scope.go.
  • Removed unused import for go.uber.org/automaxprocs in pkg/vm/engine/disttae/engine.go.

Changes walkthrough 📝

Relevant files
Bug fix
main.go
Add maxprocs logger initialization to suppress logs           

cmd/mo-service/main.go

  • Added import for go.uber.org/automaxprocs/maxprocs
  • Added init function to set maxprocs logger to a no-op function
  • +6/-0     
    compile.go
    Remove unused automaxprocs import                                               

    pkg/sql/compile/compile.go

    • Removed import for go.uber.org/automaxprocs
    +0/-1     
    scope.go
    Remove unused automaxprocs import                                               

    pkg/sql/compile/scope.go

    • Removed import for go.uber.org/automaxprocs
    +0/-1     
    engine.go
    Remove unused automaxprocs import                                               

    pkg/vm/engine/disttae/engine.go

    • Removed import for go.uber.org/automaxprocs
    +0/-1     

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

    @badboynt1
    Copy link
    Contributor Author

    重新提交这个pr,已经跑过了checkin regression

    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 changes are straightforward and localized to specific files. The removal of unused imports and the addition of a logger initialization are clear and should not require extensive testing.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Logger Suppression: The new logger initialization in main.go suppresses all logs from maxprocs. Ensure this does not hide useful debug information in production environments.

    🔒 Security concerns

    No

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label Jun 3, 2024
    @mergify mergify bot requested a review from sukki37 June 3, 2024 09:10
    @mergify mergify bot added the kind/bug Something isn't working label Jun 3, 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
    Handle potential errors from the maxprocs.Set call to aid in debugging

    To ensure that the maxprocs.Set call is effective, it would be better to handle any
    potential errors returned by the function. This can help in debugging if the setting of
    max procs fails for any reason.

    cmd/mo-service/main.go [76]

    -maxprocs.Set(maxprocs.Logger(func(string, ...interface{}) {}))
    +if _, err := maxprocs.Set(maxprocs.Logger(func(string, ...interface{}) {})); err != nil {
    +    // Handle the error, e.g., log it
    +    zap.L().Error("failed to set maxprocs", zap.Error(err))
    +}
     
    Suggestion importance[1-10]: 8

    Why: Handling errors from maxprocs.Set is crucial for robustness and debugging. The suggestion correctly identifies a potential issue and provides a practical improvement.

    8
    Best practice
    Move the maxprocs.Set call from the init function to the main function for better control over initialization

    The init function is a special function in Go and is executed before the main function. It
    might be more appropriate to place the maxprocs.Set call directly in the main function to
    ensure better control over the initialization sequence.

    cmd/mo-service/main.go [75-77]

    -func init() {
    -    maxprocs.Set(maxprocs.Logger(func(string, ...interface{}) {}))
    -}
    +func main() {
    +    if _, err := maxprocs.Set(maxprocs.Logger(func(string, ...interface{}) {})); err != nil {
    +        zap.L().Error("failed to set maxprocs", zap.Error(err))
    +    }
    +    if *maxProcessor > 0 {
    +        struntime.GOMAXPROCS(*maxProcessor)
    +    }
     
    Suggestion importance[1-10]: 7

    Why: Placing maxprocs.Set in main rather than init provides better control and explicit error handling. This is a good practice, especially for initialization that might fail or needs specific handling.

    7

    @mergify mergify bot merged commit 91b0ae0 into matrixorigin:1.2-dev Jun 3, 2024
    17 of 18 checks passed
    @badboynt1 badboynt1 deleted the auto1.2 branch June 4, 2024 00:52
    @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/XS Denotes a PR that changes [1, 9] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    6 participants