Triple client&server api #2502
Triple client&server api #2502
Conversation
chickenlj
commented
Nov 13, 2023
•
chickenlj
commented
- ConsumerConfig and ProviderConfig serve as the default configuration across services.
- Both the client and server have the same configuration hierarchy.
- dubbo.NewInstance(InstanceOption)
- instance.NewServer(ServerOption) / instance.NewClient(ClientOption)
- server.RegisterService(ServiceOption) / client.GetGreetService(ReferenceOption)
- greetService.greet(CallOption)
| test.justify(t, newOpts) | ||
| } | ||
| } | ||
| //func TestWithURL(t *testing.T) { |
There was a problem hiding this comment.
After merging this PR, we can modify this test file.
| "reflect" | ||
| ) | ||
|
|
||
| func CopyFields(sourceValue reflect.Value, targetValue reflect.Value) { |
There was a problem hiding this comment.
Java-Style but very efficient code :)
client/client.go
Outdated
| @@ -45,7 +47,7 @@ type ClientInfo struct { | |||
| Meta map[string]interface{} | |||
| } | |||
|
|
|||
| func (cli *Client) call(ctx context.Context, paramsRawVals []interface{}, interfaceName, methodName, callType string, opts ...CallOption) (protocol.Result, error) { | |||
| func (cli *Client) call(ctx context.Context, paramsRawVals []interface{}, interfaceName, methodName, group, version, callType string, opts ...CallOption) (protocol.Result, error) { | |||
There was a problem hiding this comment.
We need to consider that whether group and version should be required fields.
Since this function is relevant to non-idl API, we need to keep it simple.
For instance, if users do not need to configure group and version,
cli.CallUnary(ctx, req, resp, interfaceName, methodName, "", "")
Empty strings are very redundant.
I prefer to use CallOption to carry group and version.
There was a problem hiding this comment.
Good point. I was also thinking of using CallOption in the first place. I will move group and version to CallOption later.
There was a problem hiding this comment.
Putting group and version into CallOption has other issues. But still it's currently the best way I can think of.
client/client.go
Outdated
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return res.Result(), res.Error() | ||
| } | ||
|
|
||
| func (cli *Client) Init(info *ClientInfo) error { | ||
| func (cli *Client) Init(interfaceName string, info *ClientInfo, opts ...ReferenceOption) (string, string, error) { |
There was a problem hiding this comment.
ClientInfo contains InterfaceName. And we also need to consider about non-idl API style since this function is also important.
# Conflicts: # go.sum
|
Kudos, SonarCloud Quality Gate passed!
|
| if err != nil { | ||
| return nil, err | ||
| } | ||
| rawStream := stream.(*triple_protocol.ServerStreamForClient) | ||
| return &HealthWatchClient{rawStream}, nil | ||
| } | ||
|
|
||
| func appendGroupVersion(opts []client.CallOption, c *HealthImpl) []client.CallOption { |
There was a problem hiding this comment.
What if users passing WithGroup and WithVersion directly? Should we set priority or just ignore?
There was a problem hiding this comment.
Yeah, I have the same concern when implementing this feature.
| @@ -267,9 +273,9 @@ func WithLoadBalanceP2C() ReferenceOption { | |||
| } | |||
| } | |||
|
|
|||
| func WithLoadBalanceXDSRingHash() ReferenceOption { | |||
| func WithLoadBalance(lb string) ReferenceOption { | |||
There was a problem hiding this comment.
Not only do we need to enumerate, but we also need to leave a generic configuration API?
There was a problem hiding this comment.
Yes, I realize this is necessary when developing samples. This becomes especially useful when considering Dubbo's high extensibility, we need to make sure our API can support user's self-extended LoadBalance policies.
|
I would merge this PR and do some fine tuning. |
Thanks. Please remember to file new issues for todos recognized. |
