Copilot commented on code in PR #3225:
URL: https://github.com/apache/dubbo-go/pull/3225#discussion_r2867638000
##########
cluster/directory/static/directory.go:
##########
@@ -45,7 +47,11 @@ func NewDirectory(invokers []protocolbase.Invoker)
*directory {
invokers: invokers,
}
- dir.RouterChain().SetInvokers(invokers)
+ err := dir.BuildRouterChain(invokers, url)
+ if err != nil {
+ logger.Error(err)
Review Comment:
`NewDirectory` now calls `BuildRouterChain` and only logs on error, but does
not fall back to setting the router chain invokers. If
`chain.NewRouterChain(url)` fails (e.g., no router factories registered),
`dir.RouterChain()` remains the default empty `chain.RouterChain{}` with no
invokers, so `List()` will route against an empty chain and can return an empty
slice even though `dir.invokers` is populated. Consider always calling
`dir.RouterChain().SetInvokers(invokers)` (or setting invokers on the existing
chain) as a fallback when `BuildRouterChain` fails, so behavior matches the
previous implementation when routing is unavailable.
```suggestion
logger.Error(err)
// Fallback: ensure any existing router chain still sees the
invokers
if routerChain := dir.RouterChain(); routerChain != nil {
routerChain.SetInvokers(invokers)
}
```
##########
cluster/directory/static/directory.go:
##########
@@ -45,7 +47,11 @@ func NewDirectory(invokers []protocolbase.Invoker)
*directory {
invokers: invokers,
}
- dir.RouterChain().SetInvokers(invokers)
+ err := dir.BuildRouterChain(invokers, url)
+ if err != nil {
+ logger.Error(err)
Review Comment:
`BuildRouterChain` returns an error for `len(invokers)==0`, and
`NewDirectory` logs it as an error. If `NewDirectory` can be called with an
empty invoker list (e.g., during startup or transient states), this will
introduce noisy error logs for a non-exceptional condition. Consider skipping
router-chain building (and/or logging at debug level) when `len(invokers)==0`.
```suggestion
if len(invokers) > 0 {
if err := dir.BuildRouterChain(invokers, url); err != nil {
logger.Error(err)
}
```
##########
cluster/cluster/failover/cluster_test.go:
##########
@@ -33,6 +33,7 @@ import (
clusterpkg "dubbo.apache.org/dubbo-go/v3/cluster/cluster"
"dubbo.apache.org/dubbo-go/v3/cluster/directory/static"
"dubbo.apache.org/dubbo-go/v3/cluster/loadbalance/random"
+ _ "dubbo.apache.org/dubbo-go/v3/cluster/router/tag"
Review Comment:
The new blank import of `cluster/router/tag` is duplicated within the same
`failover` test package (it’s also imported in `cluster_invoker_test.go`). Only
one blank import is needed per package; consider keeping it in a single shared
`_test.go` (e.g., `init_test.go`) to avoid repetition.
```suggestion
```
--
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]
