Skip to content

Commit 009eb10

Browse files
committed
refactor(triple): upgrade Client to service-level abstraction
- 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>
1 parent 489ee6a commit 009eb10

7 files changed

Lines changed: 139 additions & 247 deletions

File tree

protocol/triple/client.go

Lines changed: 13 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424
"fmt"
2525
"net"
2626
"net/http"
27-
"reflect"
2827
"strings"
2928
"time"
3029
)
@@ -57,66 +56,41 @@ const (
5756
// callUnary, callClientStream, callServerStream, callBidiStream.
5857
// A Reference has a clientManager.
5958
type clientManager struct {
60-
isIDL bool
61-
// triple_protocol clients, key is method name
62-
triClients map[string]*tri.Client
59+
isIDL bool
60+
triClient *tri.Client
6361
}
6462

6563
// TODO: code a triple client between clientManager and triple_protocol client
6664
// TODO: write a NewClient for triple client
6765

68-
func (cm *clientManager) getClient(method string) (*tri.Client, error) {
69-
triClient, ok := cm.triClients[method]
70-
if !ok {
71-
return nil, fmt.Errorf("missing triple client for method: %s", method)
72-
}
73-
return triClient, nil
74-
}
75-
7666
func (cm *clientManager) callUnary(ctx context.Context, method string, req, resp any) error {
77-
triClient, err := cm.getClient(method)
78-
if err != nil {
79-
return err
80-
}
8167
triReq := tri.NewRequest(req)
8268
triResp := tri.NewResponse(resp)
83-
if err := triClient.CallUnary(ctx, triReq, triResp); err != nil {
69+
if err := cm.triClient.CallUnary(ctx, triReq, triResp, method); err != nil {
8470
return err
8571
}
8672
return nil
8773
}
8874

8975
func (cm *clientManager) callClientStream(ctx context.Context, method string) (any, error) {
90-
triClient, err := cm.getClient(method)
91-
if err != nil {
92-
return nil, err
93-
}
94-
stream, err := triClient.CallClientStream(ctx)
76+
stream, err := cm.triClient.CallClientStream(ctx, method)
9577
if err != nil {
9678
return nil, err
9779
}
9880
return stream, nil
9981
}
10082

10183
func (cm *clientManager) callServerStream(ctx context.Context, method string, req any) (any, error) {
102-
triClient, err := cm.getClient(method)
103-
if err != nil {
104-
return nil, err
105-
}
10684
triReq := tri.NewRequest(req)
107-
stream, err := triClient.CallServerStream(ctx, triReq)
85+
stream, err := cm.triClient.CallServerStream(ctx, triReq, method)
10886
if err != nil {
10987
return nil, err
11088
}
11189
return stream, nil
11290
}
11391

11492
func (cm *clientManager) callBidiStream(ctx context.Context, method string) (any, error) {
115-
triClient, err := cm.getClient(method)
116-
if err != nil {
117-
return nil, err
118-
}
119-
stream, err := triClient.CallBidiStream(ctx)
93+
stream, err := cm.triClient.CallBidiStream(ctx, method)
12094
if err != nil {
12195
return nil, err
12296
}
@@ -282,41 +256,16 @@ func newClientManager(url *common.URL) (*clientManager, error) {
282256
baseTriURL = httpPrefix + baseTriURL
283257
}
284258

285-
triClients := make(map[string]*tri.Client)
286-
287-
if len(url.Methods) != 0 {
288-
for _, method := range url.Methods {
289-
triURL, err := joinPath(baseTriURL, url.Interface(), method)
290-
if err != nil {
291-
return nil, fmt.Errorf("JoinPath failed for base %s, interface %s, method %s", baseTriURL, url.Interface(), method)
292-
}
293-
triClient := tri.NewClient(httpClient, triURL, cliOpts...)
294-
triClients[method] = triClient
295-
}
296-
} else {
297-
// This branch is for the non-IDL mode, where we pass in the service solely
298-
// for the purpose of using reflection to obtain all methods of the service.
299-
// There might be potential for optimization in this area later on.
300-
service, ok := url.GetAttribute(constant.RpcServiceKey)
301-
if !ok {
302-
return nil, fmt.Errorf("triple clientmanager can't get methods")
303-
}
304-
305-
serviceType := reflect.TypeOf(service)
306-
for i := range serviceType.NumMethod() {
307-
methodName := serviceType.Method(i).Name
308-
triURL, err := joinPath(baseTriURL, url.Interface(), methodName)
309-
if err != nil {
310-
return nil, fmt.Errorf("JoinPath failed for base %s, interface %s, method %s", baseTriURL, url.Interface(), methodName)
311-
}
312-
triClient := tri.NewClient(httpClient, triURL, cliOpts...)
313-
triClients[methodName] = triClient
314-
}
259+
triURL, err := joinPath(baseTriURL, url.Interface())
260+
if err != nil {
261+
return nil, fmt.Errorf("JoinPath failed for base %s, interface %s", baseTriURL, url.Interface())
315262
}
316263

264+
triClient := tri.NewClient(httpClient, triURL, cliOpts...)
265+
317266
return &clientManager{
318-
isIDL: isIDL,
319-
triClients: triClients,
267+
isIDL: isIDL,
268+
triClient: triClient,
320269
}, nil
321270
}
322271

protocol/triple/client_test.go

Lines changed: 16 additions & 113 deletions
Original file line numberDiff line numberDiff line change
@@ -74,12 +74,7 @@ func TestClientManager_HTTP2AndHTTP3(t *testing.T) {
7474
// If successfully created, verify the client manager
7575
assert.NotNil(t, clientManager)
7676
assert.True(t, clientManager.isIDL)
77-
assert.NotEmpty(t, clientManager.triClients)
78-
79-
// Verify that the client for the specific method exists
80-
client, exists := clientManager.triClients["testMethod"]
81-
assert.True(t, exists)
82-
assert.NotNil(t, client)
77+
assert.NotNil(t, clientManager.triClient)
8378
}
8479

8580
func TestDualTransport(t *testing.T) {
@@ -98,104 +93,18 @@ func TestDualTransport(t *testing.T) {
9893
assert.True(t, ok, "transport should implement http.RoundTripper")
9994
}
10095

101-
func TestClientManager_GetClient(t *testing.T) {
102-
tests := []struct {
103-
desc string
104-
cm *clientManager
105-
method string
106-
expectErr bool
107-
}{
108-
{
109-
desc: "method exists",
110-
cm: &clientManager{
111-
triClients: map[string]*tri.Client{
112-
"TestMethod": tri.NewClient(&http.Client{}, "http://localhost:8080/test"),
113-
},
114-
},
115-
method: "TestMethod",
116-
expectErr: false,
117-
},
118-
{
119-
desc: "method not exists",
120-
cm: &clientManager{
121-
triClients: map[string]*tri.Client{
122-
"TestMethod": tri.NewClient(&http.Client{}, "http://localhost:8080/test"),
123-
},
124-
},
125-
method: "NonExistMethod",
126-
expectErr: true,
127-
},
128-
{
129-
desc: "empty triClients",
130-
cm: &clientManager{
131-
triClients: map[string]*tri.Client{},
132-
},
133-
method: "AnyMethod",
134-
expectErr: true,
135-
},
136-
}
137-
138-
for _, test := range tests {
139-
t.Run(test.desc, func(t *testing.T) {
140-
client, err := test.cm.getClient(test.method)
141-
if test.expectErr {
142-
require.Error(t, err)
143-
assert.Nil(t, client)
144-
assert.Contains(t, err.Error(), "missing triple client")
145-
} else {
146-
require.NoError(t, err)
147-
assert.NotNil(t, client)
148-
}
149-
})
150-
}
151-
}
152-
15396
func TestClientManager_Close(t *testing.T) {
15497
cm := &clientManager{
155-
isIDL: true,
156-
triClients: map[string]*tri.Client{
157-
"Method1": tri.NewClient(&http.Client{}, "http://localhost:8080/test1"),
158-
"Method2": tri.NewClient(&http.Client{}, "http://localhost:8080/test2"),
159-
},
98+
isIDL: true,
99+
triClient: tri.NewClient(&http.Client{}, "http://localhost:8080/test"),
160100
}
161101

162102
err := cm.close()
163103
require.NoError(t, err)
164104
}
165105

166-
func TestClientManager_CallMethods_MissingClient(t *testing.T) {
167-
cm := &clientManager{
168-
triClients: map[string]*tri.Client{},
169-
}
170-
ctx := context.Background()
171-
172-
t.Run("callUnary missing client", func(t *testing.T) {
173-
err := cm.callUnary(ctx, "NonExist", nil, nil)
174-
require.Error(t, err)
175-
assert.Contains(t, err.Error(), "missing triple client")
176-
})
177-
178-
t.Run("callClientStream missing client", func(t *testing.T) {
179-
stream, err := cm.callClientStream(ctx, "NonExist")
180-
require.Error(t, err)
181-
assert.Nil(t, stream)
182-
assert.Contains(t, err.Error(), "missing triple client")
183-
})
184-
185-
t.Run("callServerStream missing client", func(t *testing.T) {
186-
stream, err := cm.callServerStream(ctx, "NonExist", nil)
187-
require.Error(t, err)
188-
assert.Nil(t, stream)
189-
assert.Contains(t, err.Error(), "missing triple client")
190-
})
191-
192-
t.Run("callBidiStream missing client", func(t *testing.T) {
193-
stream, err := cm.callBidiStream(ctx, "NonExist")
194-
require.Error(t, err)
195-
assert.Nil(t, stream)
196-
assert.Contains(t, err.Error(), "missing triple client")
197-
})
198-
}
106+
// TestClientManager_CallMethods_MissingClient removed - no longer applicable
107+
// in the service-level client architecture where all methods share a single triClient.
199108

200109
func Test_genKeepAliveOptions(t *testing.T) {
201110
defaultInterval, _ := time.ParseDuration(constant.DefaultKeepAliveInterval)
@@ -359,16 +268,17 @@ func Test_newClientManager_Serialization(t *testing.T) {
359268
}
360269

361270
func Test_newClientManager_NoMethods(t *testing.T) {
362-
// Test when url has no methods and no RpcServiceKey attribute
271+
// Test when url has no methods - in service-level client architecture,
272+
// this is valid as the client is created at service level, not method level
363273
url := common.NewURLWithOptions(
364274
common.WithLocation("localhost:20000"),
365275
common.WithPath("com.example.TestService"),
366276
)
367277

368278
cm, err := newClientManager(url)
369-
require.Error(t, err)
370-
assert.Nil(t, cm)
371-
assert.Contains(t, err.Error(), "can't get methods")
279+
require.NoError(t, err, "service-level client should be created even without method list")
280+
assert.NotNil(t, cm)
281+
assert.NotNil(t, cm.triClient, "triClient should be created at service level")
372282
}
373283

374284
func Test_newClientManager_WithMethods(t *testing.T) {
@@ -381,10 +291,7 @@ func Test_newClientManager_WithMethods(t *testing.T) {
381291
cm, err := newClientManager(url)
382292
require.NoError(t, err)
383293
assert.NotNil(t, cm)
384-
assert.Len(t, cm.triClients, 3)
385-
assert.Contains(t, cm.triClients, "Method1")
386-
assert.Contains(t, cm.triClients, "Method2")
387-
assert.Contains(t, cm.triClients, "Method3")
294+
assert.NotNil(t, cm.triClient, "triClient should be created")
388295
}
389296

390297
func Test_newClientManager_WithGroupAndVersion(t *testing.T) {
@@ -475,8 +382,8 @@ func Test_newClientManager_WithRpcService(t *testing.T) {
475382
cm, err := newClientManager(url)
476383
require.NoError(t, err)
477384
assert.NotNil(t, cm)
478-
// Should have methods from mockService (Reference, TestMethod1, TestMethod2)
479-
assert.GreaterOrEqual(t, len(cm.triClients), 2)
385+
// In service-level client architecture, a single triClient is created
386+
assert.NotNil(t, cm.triClient, "triClient should be created for non-IDL mode")
480387
}
481388

482389
func TestDualTransport_Structure(t *testing.T) {
@@ -552,7 +459,7 @@ func Test_newClientManager_URLPrefixHandling(t *testing.T) {
552459
cm, err := newClientManager(url)
553460
require.NoError(t, err)
554461
assert.NotNil(t, cm)
555-
assert.Len(t, cm.triClients, 1)
462+
assert.NotNil(t, cm.triClient, "triClient should be created")
556463
})
557464
}
558465
}
@@ -635,12 +542,8 @@ func Test_newClientManager_MultipleMethods(t *testing.T) {
635542
cm, err := newClientManager(url)
636543
require.NoError(t, err)
637544
assert.NotNil(t, cm)
638-
assert.Len(t, cm.triClients, len(methods))
639-
640-
for _, method := range methods {
641-
_, exists := cm.triClients[method]
642-
assert.True(t, exists, "method %s should exist", method)
643-
}
545+
// In service-level client architecture, a single triClient handles all methods
546+
assert.NotNil(t, cm.triClient, "triClient should be created to handle all methods")
644547
}
645548

646549
func Test_newClientManager_InterfaceName(t *testing.T) {

0 commit comments

Comments
 (0)