Copilot commented on code in PR #3326:
URL: https://github.com/apache/dubbo-go/pull/3326#discussion_r3254194122
##########
protocol/jsonrpc/jsonrpc_protocol.go:
##########
@@ -80,18 +79,17 @@ func (jp *JsonrpcProtocol) Export(invoker base.Invoker)
base.Exporter {
// Refer a remote JSON PRC service from registry
func (jp *JsonrpcProtocol) Refer(url *common.URL) base.Invoker {
- // TODO: Temporary compatibility with old APIs, can be removed later
- rt := config.GetConsumerConfig().RequestTimeout
+ rt := global.DefaultConsumerConfig().RequestTimeout
if consumerConfRaw, ok := url.GetAttribute(constant.ConsumerConfigKey);
ok {
- if consumerConf, ok :=
consumerConfRaw.(*global.ConsumerConfig); ok {
+ if consumerConf, ok :=
consumerConfRaw.(*global.ConsumerConfig); ok && consumerConf.RequestTimeout !=
"" {
rt = consumerConf.RequestTimeout
Review Comment:
`global.DefaultConsumerConfig()` allocates a new `ConsumerConfig` (including
`References` map) on every `Refer` call, but here it’s only used to read the
default timeout string. Consider avoiding per-invoker allocations by deferring
the call until needed (only when neither URL timeout nor ConsumerConfig
attribute provides a value), or by caching just the default timeout string at
package scope / exposing a lightweight `global.DefaultRequestTimeout` constant.
##########
protocol/dubbo/dubbo_invoker.go:
##########
@@ -57,10 +56,9 @@ type DubboInvoker struct {
// NewDubboInvoker constructor
func NewDubboInvoker(url *common.URL, client *remoting.ExchangeClient)
*DubboInvoker {
- // TODO: Temporary compatibility with old APIs, can be removed later
- rt := config.GetConsumerConfig().RequestTimeout
+ rt := global.DefaultConsumerConfig().RequestTimeout
if consumerConfRaw, ok := url.GetAttribute(constant.ConsumerConfigKey);
ok {
- if consumerConf, ok :=
consumerConfRaw.(*global.ConsumerConfig); ok {
+ if consumerConf, ok :=
consumerConfRaw.(*global.ConsumerConfig); ok && consumerConf.RequestTimeout !=
"" {
rt = consumerConf.RequestTimeout
}
Review Comment:
`global.DefaultConsumerConfig()` creates a new config (and allocates a
`References` map) each time `NewDubboInvoker` is called, but this code only
needs the default timeout string. To reduce allocations when creating many
invokers, consider computing the default timeout lazily (only if URL param and
consumer attribute are absent) or caching the default timeout string in a
package-level variable.
--
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]
