nanjiek commented on code in PR #3034:
URL: https://github.com/apache/dubbo-go/pull/3034#discussion_r2442463029
##########
server/server.go:
##########
@@ -39,13 +39,21 @@ import (
)
// proServices are for internal services
-var proServices = make([]*InternalService, 0, 16)
-var proLock sync.Mutex
+var internalProServices = make([]*InternalService, 0, 16)
+var internalProLock sync.Mutex
type Server struct {
cfg *ServerOptions
- svcOptsMap sync.Map
+ mu sync.RWMutex
+ // key: *ServiceOptions, value: *common.ServiceInfo
+ //proServices map[string]common.RPCService
+ // change any to *common.ServiceInfo @see config/service.go
+ svcOptsMap map[string]*ServiceOptions
+ // key is interface name, value is *ServiceOptions
+ interfaceNameServices map[string]*ServiceOptions
+ // indicate whether the server is already started
+ serve bool
}
Review Comment:
The introduction of mu and the switch from sync.Map to map + RWMutex require
stress testing to verify concurrency performance under load.
##########
protocol/grpc/server.go:
##########
@@ -150,22 +151,35 @@ func (s *Server) Start(url *common.URL) {
s.grpcServer = server
go func() {
- providerServices := config.GetProviderConfig().Services
-
- if len(providerServices) == 0 {
- panic("provider service map is null")
- }
- // wait all exporter ready , then set proxy impl and grpc
registerService
+ providerServices := s.getProviderServices(url)
+ // wait all exporter ready, then set proxy impl and grpc
registerService
waitGrpcExporter(providerServices)
registerService(providerServices, server)
reflection.Register(server)
-
- if err = server.Serve(lis); err != nil {
+ if err := server.Serve(lis); err != nil {
logger.Errorf("server serve failed with err: %v", err)
}
}()
}
+// getProviderServices retrieves provider services from config or URL
attributes
+func (s *Server) getProviderServices(url *common.URL)
map[string]*global.ServiceConfig {
+ providerServices := config.GetProviderConfig().Services
+ if len(providerServices) > 0 {
+ return convertServiceMap(providerServices)
+ }
+ // TODO #2741 old config compatibility
+ providerConfRaw, ok := url.GetAttribute(constant.ProviderConfigKey)
+ if !ok {
+ panic("no provider service found")
+ }
+ providerConf, ok := providerConfRaw.(*global.ProviderConfig)
+ if !ok || providerConf == nil {
+ panic("illegal provider config")
+ }
+ return providerConf.Services
Review Comment:
The code could be more concise.
```suggestion
if providerConfRaw, ok := url.GetAttribute(constant.ProviderConfigKey); ok {
if providerConf, ok := providerConfRaw.(*global.ProviderConfig); ok &&
providerConf != nil {
return providerConf.Services
}
panic("illegal provider config")
}
panic("no provider service found")
```
##########
protocol/triple/dubbo3_invoker.go:
##########
@@ -80,7 +80,11 @@ func NewDubbo3Invoker(url *common.URL) (*DubboInvoker,
error) {
// for triple pb serialization. The bean name from provider is the
provider reference key,
// 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)
+ if rpcService, ok := url.GetAttribute(constant.RpcServiceKey); ok {
+ consumerService = rpcService
Review Comment:
It‘s the same problem I found before.
##########
protocol/grpc/client.go:
##########
@@ -139,8 +139,12 @@ func NewClient(url *common.URL) (*Client, error) {
}
key := url.GetParam(constant.InterfaceKey, "")
- impl := config.GetConsumerServiceByInterfaceName(key)
- invoker := getInvoker(impl, conn)
+ //TODO: Temporary compatibility with old APIs, can be removed later
+ consumerService := config.GetConsumerServiceByInterfaceName(key)
+ if rpcService, ok := url.GetAttribute(constant.RpcServiceKey); ok {
+ consumerService = rpcService
Review Comment:
The type assertion to interface{} is unnecessary because every type in Go
implements interface{}.
This means the assertion will always succeed, and ok will always be true.
##########
protocol/grpc/client.go:
##########
@@ -139,8 +139,12 @@ func NewClient(url *common.URL) (*Client, error) {
}
key := url.GetParam(constant.InterfaceKey, "")
- impl := config.GetConsumerServiceByInterfaceName(key)
- invoker := getInvoker(impl, conn)
+ //TODO: Temporary compatibility with old APIs, can be removed later
+ consumerService := config.GetConsumerServiceByInterfaceName(key)
+ if rpcService, ok := url.GetAttribute(constant.RpcServiceKey); ok {
+ consumerService = rpcService
Review Comment:
Don't use interface{},please use any.Starting from Go 1.18, which added
support for generics, the Go community and the official style guidelines
recommend using any rather than interface{} when you want to express that a
value can be of any type. Although both are functionally identical (any is just
a type alias for interface{}), any reads more naturally in generic constraints
and makes the code easier to understand.
--
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]
