Skip to content

refactor(triple): upgrade triple client to service-level abstraction#3086

Merged
AlexStocks merged 2 commits intoapache:developfrom
Aetherance:feature/triple-client-pool
Jan 20, 2026
Merged

refactor(triple): upgrade triple client to service-level abstraction#3086
AlexStocks merged 2 commits intoapache:developfrom
Aetherance:feature/triple-client-pool

Conversation

@Aetherance
Copy link
Contributor

@Aetherance Aetherance commented Nov 23, 2025

Background

Currently, triple_protocol.Client is designed as a procedure-level client (one client instance per RPC method), which leads to:

  • High memory overhead: O(methods) client instances per service
  • Complex management: clientManager maintains a map[string]*Client to track method-level clients
  • Duplicated logic: Separate code paths for IDL and non-IDL modes, including reflection-based method discovery

This design differs from the gRPC client model, which uses a service-level client, where a single client instance manages all RPC methods of a service.


Changes

This PR refactors triple_protocol.Client to a service-level architecture.

  1. Core API Changes (protocol/triple/triple_protocol/client.go)
  • Add method string parameter to all Call* methods:

    • CallUnary(ctx, req, res, method)
    • CallClientStream(ctx, method)
    • CallServerStream(ctx, req, method)
    • CallBidiStream(ctx, method)
  • Introduce buildMethodLevelReqSpec() to construct method-level request specs from a service-level spec

  • Add normalizeClientProcedure() for consistent URL path handling

  1. Client Manager Simplification (protocol/triple/client.go)

Refactor clientManager from a method-level to a service-level structure:

// Before
type clientManager struct {
    isIDL      bool
    triClients map[string]*tri.Client
}

// After
type clientManager struct {
    isIDL     bool
    triClient *tri.Client
}
  • Remove the getClient(method) helper
  • Unify IDL and non-IDL client creation logic
  • Update all call paths to explicitly pass the method parameter
  1. Bug Fix (protocol/triple/triple_protocol/duplex_http_call.go)
  • Bind url.Path = spec.Procedure to ensure requests are routed to the correct RPC method
  • Improves correctness in duplex streaming scenarios
  1. Test Updates
  • Update test cases to align with the service-level client model
  • Ensure coverage for updated client and manager logic

Impact

Memory & Performance

  • Client instances per service: O(methods) → O(1)
  • Code changes: Net -108 lines (7 files changed, 133 insertions, 241 deletions)

API Compatibility

  • Breaking Change: triple_protocol.Client APIs now require an explicit method parameter
  • Limited impact: triple_protocol is primarily an internal API
  • Higher-level APIs unchanged: client.Client and client.Connection remain compatible

Migration Guide

For direct users of triple_protocol.Client (rare):

// Before
client := tri.NewClient(httpClient, "http://host/service/Method", opts...)
err := client.CallUnary(ctx, req, res)

// After
client := tri.NewClient(httpClient, "http://host/service", opts...)
err := client.CallUnary(ctx, req, res, "Method")

@Aetherance
Copy link
Contributor Author

issue #3087

@CAICAIIs
Copy link
Contributor

please fix the code format

@CAICAIIs
Copy link
Contributor

Maybe it would be better to put TriClientPool in a separate new file instead of cramming it into the existing client.go?

@Aetherance Aetherance marked this pull request as draft November 24, 2025 07:41
@Aetherance
Copy link
Contributor Author

This PR is still in progress. I’ll update it once it's ready for review.

@Alanxtl Alanxtl linked an issue Nov 24, 2025 that may be closed by this pull request
2 tasks
@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 1fd05cc to 5caf993 Compare November 25, 2025 07:46
@marsevilspirit
Copy link
Member

Fix ci error.

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 5caf993 to 0588f18 Compare November 27, 2025 04:58
@codecov-commenter
Copy link

codecov-commenter commented Nov 27, 2025

Codecov Report

❌ Patch coverage is 52.23881% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 48.04%. Comparing base (60d1c2a) to head (a926b9c).
⚠️ Report is 713 commits behind head on develop.

Files with missing lines Patch % Lines
protocol/triple/triple_protocol/client.go 61.70% 9 Missing and 9 partials ⚠️
...roto/connect/ping/v1/pingv1connect/ping.connect.go 0.00% 8 Missing ⚠️
protocol/triple/client.go 40.00% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #3086      +/-   ##
===========================================
+ Coverage    46.76%   48.04%   +1.27%     
===========================================
  Files          295      460     +165     
  Lines        17172    33145   +15973     
===========================================
+ Hits          8031    15924    +7893     
- Misses        8287    15926    +7639     
- Partials       854     1295     +441     

☔ 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.

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 0aa8efa to f99ee04 Compare November 27, 2025 05:29
@Aetherance Aetherance marked this pull request as ready for review November 28, 2025 05:07
@Aetherance
Copy link
Contributor Author

@marsevilspirit review

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch 2 times, most recently from f5b2a76 to f3f503e Compare November 28, 2025 12:56
@Aetherance
Copy link
Contributor Author

@marsevilspirit done

@marsevilspirit
Copy link
Member

Write a performance test to demonstrate that your change can improve the throughput of triple.

Copy link
Contributor

@nanjiek nanjiek left a comment

Choose a reason for hiding this comment

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

Why delete the error message about base and interface?

@Aetherance
Copy link
Contributor Author

Thanks for reviewing. I will fix later

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from f3f503e to 78dba57 Compare November 30, 2025 09:30
Copy link
Contributor

@Alanxtl Alanxtl left a comment

Choose a reason for hiding this comment

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

protocol/triple/client_pool.go needs sound unit tests, please write a test file for it

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 78dba57 to e908b8a Compare December 5, 2025 12:58
@Aetherance Aetherance changed the title feat(triple): implement TriClientPool feat(triple): Implement TriClientPool and Delay triple client Method Binding Dec 12, 2025
@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from b2d8371 to 8251f35 Compare December 14, 2025 08:30
@Aetherance Aetherance marked this pull request as draft December 14, 2025 12:17
@AlexStocks AlexStocks added 3.3.2 version 3.3.2 and removed 3.3.1 labels Dec 18, 2025
@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 8251f35 to 01443d1 Compare December 18, 2025 17:51
@Aetherance Aetherance marked this pull request as ready for review December 19, 2025 12:58
@Alanxtl
Copy link
Contributor

Alanxtl commented Dec 24, 2025

benchmark 能不能加 大流量 和 并发量高一些 的场景,看一下这两种场景中修改前后的benchmark结果

@marsevilspirit
Copy link
Member

Performance test reference: https://github.com/marsevilspirit/dubbo-go-benchmark

@Aetherance
Copy link
Contributor Author

benchmark 能不能加 大流量 和 并发量高一些 的场景,看一下这两种场景中修改前后的benchmark结果

ok

@Aetherance
Copy link
Contributor Author

Performance test reference: https://github.com/marsevilspirit/dubbo-go-benchmark

got it

@marsevilspirit
Copy link
Member

Is this PR process still ongoing?

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch 4 times, most recently from e40c041 to 009eb10 Compare January 10, 2026 19:48
@Aetherance Aetherance changed the title feat(triple): Implement TriClientPool and Delay triple client Method Binding refactor(triple): upgrade triple client to service-level abstraction Jan 10, 2026
@Aetherance
Copy link
Contributor Author

This PR originally implemented a TriClientPool, but the design was refactored to a service-level client.
This simplifies management and reduces memory overhead.

@Aetherance
Copy link
Contributor Author

@marsevilspirit

@AlexStocks
Copy link
Contributor

@Aetherance please fix the file confliction
@marsevilspirit please review the latest changes.

@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 009eb10 to 9b1537d Compare January 19, 2026 11:05
@Aetherance
Copy link
Contributor Author

@AlexStocks confliction fixed

- Change triple_protocol.Client from procedure-level to service-level
- Simplify clientManager from map[string]*Client to single *Client
- Unify IDL and non-IDL client creation logic
- Fix connect-go interoperability by updating ping.connect.go

Reduces client instances from O(methods) to O(1) per service.
Net: -16 lines of code.

Signed-off-by: Aetherance <inkToAether@gmail.com>
@Aetherance Aetherance force-pushed the feature/triple-client-pool branch from 9b1537d to 0762edb Compare January 19, 2026 13:00
Add handling for empty strings and multiple slashes in normalizeClientProcedure.
@sonarqubecloud
Copy link

@AlexStocks AlexStocks merged commit 3de582d into apache:develop Jan 20, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Implement Triple Client Pool

8 participants