Copilot commented on code in PR #995:
URL: https://github.com/apache/dubbo-go-pixiu/pull/995#discussion_r3480483135
##########
admin/core/xds.go:
##########
@@ -92,13 +91,13 @@ func StartxDsServer() error {
config := GenerateSnapshotPixiu()
if err := config.Consistent(); err != nil {
logger.Errorf("config inconsistency: %+v\n%+v", config, err)
- os.Exit(1)
+ return fmt.Errorf("config inconsistency: %w", err)
}
Review Comment:
StartxDsServer() logs the full inconsistency (including the snapshot
contents) and also returns an error; the caller (admin/core/server.go) logs the
returned error again. This can lead to duplicate log lines and potentially very
noisy logs. Consider either removing this log here (and rely on the returned
error) or returning richer context and letting the top-level decide what/how to
log.
##########
admin/core/server.go:
##########
@@ -50,7 +50,12 @@ type server interface {
// RunServer start server
func RunServer() {
// load config
- global.VP = Viper()
+ vp, err := Viper()
+ if err != nil {
+ fmt.Printf("load config error: %v\n", err)
+ return
+ }
Review Comment:
On config load failure, RunServer() now just prints and returns. The admin
CLI (cmd/admin/admin.go) calls core.RunServer() and then blocks waiting for a
shutdown signal, so a config error can leave the process running without
starting servers (and likely exiting with code 0). Consider changing
RunServer() to return an error and have the Cobra command fail fast (e.g.,
switch to RunE and return the error) so startup failure is surfaced to
callers/automation.
##########
admin/core/xds.go:
##########
@@ -148,7 +147,9 @@ func watchConfigAndReload() {
if err != nil {
logger.Errorf("watch config error %q", err)
- panic(err)
+ // Log the error and return - don't panic in background
goroutine
+ // The server should continue running with existing config
+ return
Review Comment:
If WatchWithPrefix fails, watchConfigAndReload() now returns and permanently
stops config reloads. For transient etcd/network errors this means the xDS
server will never pick up future config changes. Consider retrying the watch
with a backoff (and also restarting the watch if the channel closes) so reload
functionality can recover.
##########
admin/core/viper.go:
##########
@@ -56,7 +56,7 @@ func Viper(path ...string) *viper.Viper {
v.SetConfigType("yaml")
err := v.ReadInConfig()
if err != nil {
- panic(fmt.Errorf("fatal error config file: %s", err))
+ return nil, fmt.Errorf("fatal error config file: %s", err)
Review Comment:
Use %w when returning the config read error so callers can unwrap/inspect it
with errors.Is / errors.As. Using %s stringifies the underlying error and loses
the original cause.
--
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]
