Copilot commented on code in PR #3211:
URL: https://github.com/apache/dubbo-go/pull/3211#discussion_r2798166749
##########
dubbo.go:
##########
@@ -35,6 +35,8 @@ import (
var (
consumerServices = map[string]*client.ClientDefinition{}
conLock sync.RWMutex
+ consumerServicesByInterfaceName = map[string]common.RPCService{}
+ conInterfaceLock sync.RWMutex
providerServices = map[string]*server.ServiceDefinition{}
Review Comment:
New global registry `consumerServicesByInterfaceName` duplicates the
existing interface-name consumer registry in `config` (e.g.,
`config/reference_config.go` registers via
`config.SetConsumerServiceByInterfaceName`). Since the invokers were switched
to `dubbo.GetConsumerServiceByInterfaceName`, consumers registered through the
config-based path won’t be discoverable. Consider delegating these dubbo-level
Set/Get APIs to the existing registry (or adding a fallback to config) or
migrating all registration sites to the dubbo-level API to keep compatibility.
##########
protocol/dubbo3/internal/server.go:
##########
@@ -27,7 +27,6 @@ import (
import (
"dubbo.apache.org/dubbo-go/v3/common"
Review Comment:
`common` is now imported but no longer used in this file after removing
`InitDubboServer`, which will cause a Go compile error. Please drop the unused
`dubbo.apache.org/dubbo-go/v3/common` import (or reintroduce a use if still
needed).
```suggestion
```
##########
dubbo.go:
##########
@@ -297,6 +299,20 @@ func SetConsumerService(svc common.RPCService) {
}
}
+// SetConsumerServiceByInterfaceName registers a consumer by interface name
for pb serialization.
+func SetConsumerServiceByInterfaceName(interfaceName string, srv
common.RPCService) {
+ conInterfaceLock.Lock()
+ defer conInterfaceLock.Unlock()
+ consumerServicesByInterfaceName[interfaceName] = srv
+}
Review Comment:
`SetConsumerServiceByInterfaceName` / `GetConsumerServiceByInterfaceName`
introduce new behavior but there are no unit tests in this package covering
registration + lookup by interface name. Please add a small test (similar to
existing tests in `dubbo_test.go`) to prevent regressions, especially around
concurrent access.
##########
protocol/triple/dubbo3_invoker.go:
##########
@@ -81,7 +82,7 @@ func NewDubbo3Invoker(url *common.URL) (*DubboInvoker, error)
{
// which can't locate the target consumer stub, so we use interface
key..
interfaceKey := url.GetParam(constant.InterfaceKey, "")
//TODO: Temporary compatibility with old APIs, can be removed later
- consumerService :=
config.GetConsumerServiceByInterfaceName(interfaceKey)
+ consumerService := dubbo.GetConsumerServiceByInterfaceName(interfaceKey)
if consumerService == nil {
if rpcService, ok := url.GetAttribute(constant.RpcServiceKey);
ok {
Review Comment:
If both the interface-name lookup and `constant.RpcServiceKey` attribute are
absent, `consumerService` stays nil and is passed into
`triple.NewTripleClient`, which is likely to panic or fail in a non-obvious
way. Please add an explicit nil check here and return a descriptive error that
tells callers how to register/provide the consumer stub.
##########
protocol/dubbo3/dubbo3_invoker.go:
##########
@@ -85,7 +86,7 @@ func NewDubboInvoker(url *common.URL) (*DubboInvoker, error) {
// which can't locate the target consumer stub, so we use interface key.
interfaceKey := url.GetParam(constant.InterfaceKey, "")
//TODO: Temporary compatibility with old APIs, can be removed later
- consumerService = config.GetConsumerServiceByInterfaceName(interfaceKey)
+ consumerService = dubbo.GetConsumerServiceByInterfaceName(interfaceKey)
if rpcService, ok := url.GetAttribute(constant.RpcServiceKey); ok {
consumerService = rpcService
}
Review Comment:
If neither the interface-name registry nor `constant.RpcServiceKey` provides
a consumer stub, `consumerService` remains nil and will be used to construct
the Triple client. Please fail fast with a clear error when the stub can’t be
resolved to avoid later panics/opaque failures.
```suggestion
}
if consumerService == nil {
return nil, errors.New("dubbo3: failed to resolve consumer stub
for interface: " + interfaceKey)
}
```
##########
protocol/grpc/client.go:
##########
@@ -143,7 +143,7 @@ func NewClient(url *common.URL) (*Client, error) {
key := url.GetParam(constant.InterfaceKey, "")
//TODO: Temporary compatibility with old APIs, can be removed later
- consumerService := config.GetConsumerServiceByInterfaceName(key)
+ consumerService := dubbo.GetConsumerServiceByInterfaceName(key)
if consumerService == nil {
if rpcService, ok := url.GetAttribute(constant.RpcServiceKey);
ok {
Review Comment:
If `dubbo.GetConsumerServiceByInterfaceName(key)` returns nil and
`constant.RpcServiceKey` isn’t set on the URL, `getInvoker(consumerService,
conn)` will panic (reflect on a nil impl). Please return a clear error when the
consumer stub can’t be resolved, and include guidance to either register via
`dubbo.SetConsumerServiceByInterfaceName` or pass the stub via
`constant.RpcServiceKey`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
