Copilot commented on code in PR #994:
URL: https://github.com/apache/dubbo-go-pixiu/pull/994#discussion_r3480477793
##########
pkg/config/xds/xds.go:
##########
@@ -69,7 +69,12 @@ func (a *Xds) createApiManager(config *model.ApiConfigSource,
logger.Errorf("can not read listener. %v", err)
return nil
}
- return apiclient.CreateEnvoyGrpcApiClient(config, node,
a.exitCh, resourceType, apiclient.WithIstioService(dubboServices...))
+ client, err := apiclient.CreateEnvoyGrpcApiClient(config, node,
a.exitCh, resourceType, apiclient.WithIstioService(dubboServices...))
+ if err != nil {
+ logger.Errorf("create envoy grpc api client error: %v",
err)
+ return nil
+ }
Review Comment:
`createApiManager` returns `nil` when `CreateEnvoyGrpcApiClient` fails, but
`Start()` unconditionally constructs `LdsManager/CdsManager` with `DiscoverApi:
a.createApiManager(...)` and then calls `Delta()` on it. If `DiscoverApi` is
nil, `l.DiscoverApi.Delta()` will panic (nil interface method call), which
defeats the goal of replacing panics with graceful error handling.
##########
pkg/config/xds/xds.go:
##########
@@ -69,7 +69,12 @@ func (a *Xds) createApiManager(config *model.ApiConfigSource,
logger.Errorf("can not read listener. %v", err)
return nil
}
- return apiclient.CreateEnvoyGrpcApiClient(config, node,
a.exitCh, resourceType, apiclient.WithIstioService(dubboServices...))
+ client, err := apiclient.CreateEnvoyGrpcApiClient(config, node,
a.exitCh, resourceType, apiclient.WithIstioService(dubboServices...))
+ if err != nil {
+ logger.Errorf("create envoy grpc api client error: %v",
err)
+ return nil
+ }
Review Comment:
This log uses `%v` but the error is wrapped with `github.com/pkg/errors`
(`errors.Wrap`). Using `%+v` preserves the wrapped stack/context and is already
used elsewhere in this file (e.g., `can not fetch lds err is %+v`).
##########
pkg/config/xds/apiclient/grpc_envoy.go:
##########
@@ -111,15 +113,16 @@ func (g *AggGrpcApiClient) init() {
if err != nil {
logger.Errorf("get cluster for init error. error=%v", err)
- panic(err)
+ return errors.Wrap(err, "get cluster for init error")
}
Review Comment:
`init()` both logs the error and returns a wrapped error. Since callers now
handle/ log the returned error, this tends to produce duplicate log lines for
the same failure; prefer returning the wrapped error and letting the top-level
caller decide how/where to log.
##########
pkg/config/xds/apiclient/grpc_envoy.go:
##########
@@ -99,9 +101,9 @@ type (
discoveryResponseHandler func(any2 []*anypb.Any)
)
-func (g *AggGrpcApiClient) init() {
+func (g *AggGrpcApiClient) init() error {
if len(g.config.ClusterName) == 0 {
- panic("should config one cluster at least")
+ return errors.New("should config one cluster at least")
}
Review Comment:
The new error message is ungrammatical and not very actionable. Consider
phrasing it as a requirement and referencing the specific config field (cluster
name) to help operators diagnose misconfiguration quickly.
##########
pkg/config/xds/apiclient/grpc_envoy.go:
##########
@@ -86,8 +86,10 @@ func CreateEnvoyGrpcApiClient(
for _, fn := range opts {
fn(v)
}
- v.init()
- return v
+ if err := v.init(); err != nil {
+ return nil, err
+ }
+ return v, nil
Review Comment:
This PR changes `CreateEnvoyGrpcApiClient`/`AggGrpcApiClient.init` from
panic to error returns, but there is no unit test asserting that
misconfiguration (e.g., empty `ClusterName`) and dependency failures return an
error without panicking. The apiclient package already has tests (e.g.,
`grpc_test.go`), so adding a focused test for the new behavior would prevent
regressions back to panic-style handling.
--
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]
