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

set srv.pu before creating hakeeper client #18672

Merged
merged 5 commits into from
Sep 12, 2024

Conversation

w-zr
Copy link
Contributor

@w-zr w-zr 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/3973

What this PR does / why we need it:

set srv.pu before creating hakeeper client


PR Type

Bug fix


Description

  • Fixed the initialization order by setting srv.pu before creating the HAKeeper client, addressing issue Add hashmap #3973.
  • Reorganized the service struct initialization to ensure all dependencies are set up correctly.
  • Removed redundant code to streamline the setup process and improve maintainability.
  • Ensured that srv.pu is configured with the lock service, HAKeeper client, query client, and UDF service.

Changes walkthrough 📝

Relevant files
Bug fix
server.go
Fix initialization order for HAKeeper client in service setup

pkg/cnservice/server.go

  • Set srv.pu before initializing the HAKeeper client.
  • Reorganized the initialization sequence of the service struct.
  • Removed redundant code and streamlined the request handler setup.
  • Ensured srv.pu is properly configured with necessary services.
  • +41/-75 

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

    Code Reorganization
    The initialization order has been changed, with srv.pu now being set before creating the HAKeeper client. This change addresses the issue #3973 but may have unintended consequences if other parts of the code rely on the previous initialization order.

    Potential Race Condition
    The srv.pu is being accessed and modified in multiple places throughout the initialization process. This could potentially lead to race conditions if any of these operations are not thread-safe.

    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Sep 10, 2024
    @mergify mergify bot requested a review from sukki37 September 10, 2024 02:59
    @mergify mergify bot added the kind/bug Something isn't working label Sep 10, 2024
    Copy link

    codiumai-pr-agent-pro bot commented Sep 10, 2024

    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
    Error handling
    Improve error handling and logging for the MO server initialization

    Consider adding error handling for the srv.initMOServer call. Currently, if an error
    occurs, it's directly returned without any logging or cleanup. Adding proper error
    handling and logging would improve the robustness of the service initialization.

    pkg/cnservice/server.go [181-183]

     if err = srv.initMOServer(ctx, srv.pu, srv.aicm); err != nil {
    -  return nil, err
    +  srv.logger.Error("Failed to initialize MO server", zap.Error(err))
    +  srv.stopper.Stop()
    +  return nil, fmt.Errorf("failed to initialize MO server: %w", err)
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling and logging for the srv.initMOServer call is crucial for debugging and ensuring proper resource cleanup, significantly improving the robustness of the service initialization process.

    9
    Resource management
    Ensure proper cleanup of resources in case of initialization errors

    Consider adding a defer statement to stop the stopper in case of any errors during
    service initialization. This ensures that resources are properly cleaned up if an
    error occurs after the stopper is created.

    pkg/cnservice/server.go [131]

     srv.stopper = stopper.NewStopper("cn-service", stopper.WithLogger(srv.logger))
    +defer func() {
    +  if err != nil {
    +    srv.stopper.Stop()
    +  }
    +}()
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Ensuring proper cleanup of resources in case of initialization errors is important for resource management and preventing potential memory leaks, making this a valuable improvement.

    8
    Best practice
    ✅ Initialize the parameter unit earlier in the service creation process
    Suggestion Impact:The suggestion was implemented by moving the srv.registerServices() call before the initialization of srv.pu.

    code diff:

    +	srv.registerServices()
     	pu := config.NewParameterUnit(
     		&cfg.Frontend,
     		nil,
    @@ -139,7 +140,6 @@
     		}})
     	frontend.InitServerVersion(pu.SV.MoVersion)
     	srv.pu = pu
    -	srv.registerServices()

    Consider initializing srv.pu earlier in the NewService function, before calling
    srv.registerServices(). This ensures that srv.pu is available for use in any methods
    called during service initialization.

    pkg/cnservice/server.go [142-145]

    +srv.pu = config.NewParameterUnit(
    +  &cfg.Frontend,
    +  nil,
    +  nil,
    +  engine.Nodes{engine.Node{
    +    Addr: srv.pipelineServiceServiceAddr(),
    +  }})
    +frontend.InitServerVersion(srv.pu.SV.MoVersion)
     srv.registerServices()
     if _, err = srv.getHAKeeperClient(); err != nil {
       return nil, err
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Initializing srv.pu earlier can improve the robustness of the service initialization by ensuring that it is available for any subsequent method calls. However, the current code does not appear to have any immediate issues, so this is more of a best practice for maintainability.

    7
    Maintainability
    Group related parameter unit initializations together for better code organization

    Consider moving the initialization of srv.pu.LockService, srv.pu.HAKeeperClient,
    srv.pu.QueryClient, srv.pu.UdfService, and srv._txnClient closer to where srv.pu is
    created. This groups related initializations together, improving code readability
    and maintainability.

    pkg/cnservice/server.go [153-157]

    +srv.pu = config.NewParameterUnit(
    +  &cfg.Frontend,
    +  nil,
    +  nil,
    +  engine.Nodes{engine.Node{
    +    Addr: srv.pipelineServiceServiceAddr(),
    +  }})
    +frontend.InitServerVersion(srv.pu.SV.MoVersion)
     srv.pu.LockService = srv.lockService
     srv.pu.HAKeeperClient = srv._hakeeperClient
     srv.pu.QueryClient = srv.queryClient
     srv.pu.UdfService = srv.udfService
     srv._txnClient = srv.pu.TxnClient
     
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Grouping related initializations together can improve code readability and maintainability, but this is a minor improvement and not critical to the functionality of the code.

    6

    @mergify mergify bot merged commit 3c6e4c5 into matrixorigin:1.2-dev Sep 12, 2024
    16 of 18 checks passed
    @w-zr w-zr deleted the fix-MOCloud-3973 branch September 20, 2024 03:39
    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/M Denotes a PR that changes [100,499] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants