Skip to content

Conversation

@hwbrzzl
Copy link
Contributor

@hwbrzzl hwbrzzl commented Jan 31, 2026

Greptile Overview

Greptile Summary

Refactored gRPC client configuration naming from clients to servers to better reflect semantic meaning - the configuration describes gRPC servers that the client connects to, not multiple clients.

Key changes:

  • Added new Connect(server string) method to replace Client(ctx, name) - removes unused ctx parameter and renames name to server for clarity
  • Deprecated Client method with v1.18 removal notice
  • Renamed internal clients map to servers map with updated mutex comment
  • Updated configuration paths from grpc.clients.* to grpc.servers.*
  • Updated receiver variable from app to r for consistency
  • Updated all tests to use new config paths
  • Updated config stub template with clearer documentation
  • Generated mock for new Connect method

The refactoring maintains backward compatibility through the deprecated Client method which delegates to Connect.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The refactoring is well-executed with proper deprecation strategy, comprehensive test updates, and maintains backward compatibility. Only issue is a minor comment typo that doesn't affect functionality.
  • No files require special attention

Important Files Changed

Filename Overview
contracts/grpc/grpc.go Added new Connect method and deprecated Client method; comment typo found
grpc/application.go Refactored internal naming from clients to servers, deprecated Client in favor of Connect, updated config paths from grpc.clients.* to grpc.servers.*
grpc/application_test.go Updated test expectations to use new config path grpc.servers.* instead of grpc.clients.*
grpc/setup/stubs.go Updated config stub template to use servers instead of clients with clearer documentation
mocks/grpc/Grpc.go Added mock implementation for new Connect method

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant App as Application
    participant Config as Config
    participant Cache as servers map
    participant GRPC as gRPC Server

    Note over Client,App: Old API (Deprecated)
    Client->>App: Client(ctx, name)
    App->>App: Connect(name)
    
    Note over Client,GRPC: New API
    Client->>App: Connect(server)
    App->>Cache: Check cached connection (RLock)
    
    alt Connection exists and healthy
        Cache-->>App: Return cached conn
        App-->>Client: Return connection
    else No connection or shutdown
        App->>Cache: Acquire write lock
        App->>Cache: Double-check pattern
        
        alt Still needs creation
            App->>Config: GetString("grpc.servers.{server}.host")
            Config-->>App: host
            App->>Config: Get("grpc.servers.{server}.interceptors")
            Config-->>App: interceptor keys
            App->>Config: Get("grpc.servers.{server}.stats_handlers")
            Config-->>App: stats handler keys
            App->>GRPC: grpc.NewClient(host, dialOpts)
            GRPC-->>App: newConn
            App->>Cache: Store connection
            Cache-->>App: Connection stored
            App-->>Client: Return new connection
        else Created by another goroutine
            Cache-->>App: Return existing conn
            App-->>Client: Return connection
        end
    end
Loading

@hwbrzzl hwbrzzl requested a review from a team as a code owner January 31, 2026 08:06

// Configure your client host and interceptors.
// Interceptors can be the group name of UnaryClientInterceptorGroups in app/grpc/kernel.go.
"clients": map[string]any{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's incorrect before. It's actually servers.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

3 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Jan 31, 2026

Codecov Report

❌ Patch coverage is 94.64286% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 70.01%. Comparing base (869f116) to head (91ee246).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
grpc/application.go 96.36% 1 Missing and 1 partial ⚠️
grpc/setup/stubs.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1363   +/-   ##
=======================================
  Coverage   70.00%   70.01%           
=======================================
  Files         284      284           
  Lines       17714    17716    +2     
=======================================
+ Hits        12401    12403    +2     
  Misses       4777     4777           
  Partials      536      536           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

@hwbrzzl hwbrzzl merged commit c66a230 into master Jan 31, 2026
17 checks passed
@hwbrzzl hwbrzzl deleted the bowen/optimize-grpc branch January 31, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants