Copilot commented on code in PR #3386:
URL: https://github.com/apache/dubbo-go/pull/3386#discussion_r3384845987
##########
global/shutdown_config.go:
##########
@@ -82,6 +92,72 @@ func DefaultShutdownConfig() *ShutdownConfig {
return cfg
}
+// Prefix dubbo.shutdown
+func (c *ShutdownConfig) Prefix() string {
+ return constant.ShutdownConfigPrefix
+}
+
+func (c *ShutdownConfig) GetTimeout() time.Duration {
+ result, err := time.ParseDuration(c.Timeout)
+ if err != nil {
+ logger.Errorf("The Timeout configuration is invalid: %s, and we
will use the default value: %s, err: %v",
+ c.Timeout,
constant.DefaultShutdownConfigTimeout.String(), err)
+ return constant.DefaultShutdownConfigTimeout
+ }
+ return result
+}
+
+func (c *ShutdownConfig) GetStepTimeout() time.Duration {
+ result, err := time.ParseDuration(c.StepTimeout)
+ if err != nil {
+ logger.Errorf("The StepTimeout configuration is invalid: %s,
and we will use the default value: %s, err: %v",
+ c.StepTimeout,
constant.DefaultShutdownConfigStepTimeout.String(), err)
+ return constant.DefaultShutdownConfigStepTimeout
+ }
+ return result
+}
+
+func (c *ShutdownConfig) GetNotifyTimeout() time.Duration {
+ result, err := time.ParseDuration(c.NotifyTimeout)
+ if err != nil {
+ logger.Errorf("The NotifyTimeout configuration is invalid: %s,
and we will use the default value: %s, err: %v",
+ c.NotifyTimeout,
constant.DefaultShutdownConfigNotifyTimeout.String(), err)
+ return constant.DefaultShutdownConfigNotifyTimeout
+ }
+ return result
+}
+
+func (c *ShutdownConfig) GetOfflineRequestWindowTimeout() time.Duration {
+ result, err := time.ParseDuration(c.OfflineRequestWindowTimeout)
+ if err != nil {
+ logger.Errorf("The OfflineRequestWindowTimeout configuration is
invalid: %s, and we will use the default value: %s, err: %v",
+ c.OfflineRequestWindowTimeout,
constant.DefaultShutdownConfigOfflineRequestWindowTimeout.String(), err)
+ return constant.DefaultShutdownConfigOfflineRequestWindowTimeout
+ }
+ return result
+}
+
+func (c *ShutdownConfig) GetConsumerUpdateWaitTime() time.Duration {
+ result, err := time.ParseDuration(c.ConsumerUpdateWaitTime)
+ if err != nil {
+ logger.Errorf("The ConsumerUpdateTimeout configuration is
invalid: %s, and we will use the default value: %s, err: %v",
+ c.ConsumerActiveCount.Load(),
constant.DefaultShutdownConfigConsumerUpdateWaitTime.String(), err)
+ return constant.DefaultShutdownConfigConsumerUpdateWaitTime
+ }
Review Comment:
GetConsumerUpdateWaitTime logs the wrong "invalid" value
(ConsumerActiveCount) and uses a %s format with a non-string, which will
produce `%!s(int32=...)` output. It should log the invalid
ConsumerUpdateWaitTime string and use the correct field name in the message.
##########
config/graceful_shutdown_test.go:
##########
@@ -17,6 +17,76 @@
package config
+import (
+ "context"
+ "testing"
+)
+
+import (
+ "github.com/stretchr/testify/assert"
+ "github.com/stretchr/testify/require"
+)
+
+import (
+ "dubbo.apache.org/dubbo-go/v3/common/constant"
+ "dubbo.apache.org/dubbo-go/v3/common/extension"
+ "dubbo.apache.org/dubbo-go/v3/filter"
+ "dubbo.apache.org/dubbo-go/v3/global"
+ "dubbo.apache.org/dubbo-go/v3/protocol/base"
+ "dubbo.apache.org/dubbo-go/v3/protocol/result"
+)
+
+type captureShutdownConfigFilter struct {
+ captured any
+}
+
+func (f *captureShutdownConfigFilter) Invoke(context.Context, base.Invoker,
base.Invocation) result.Result {
+ return &result.RPCResult{}
+}
+
+func (f *captureShutdownConfigFilter) OnResponse(context.Context,
result.Result, base.Invoker, base.Invocation) result.Result {
+ return &result.RPCResult{}
+}
+
+func (f *captureShutdownConfigFilter) Set(_ string, config any) {
+ f.captured = config
+}
+
+func TestGracefulShutdownInitPassesGlobalShutdownConfigToFilters(t *testing.T)
{
+ internalSignal := false
+ SetRootConfig(RootConfig{
+ Shutdown: &ShutdownConfig{
+ Timeout: "11s",
+ StepTimeout: "2s",
+ NotifyTimeout: "4s",
+ InternalSignal: &internalSignal,
+ },
+ })
+
+ consumerFilter := &captureShutdownConfigFilter{}
+ providerFilter := &captureShutdownConfigFilter{}
+ extension.SetFilter(constant.GracefulShutdownConsumerFilterKey, func()
filter.Filter {
+ return consumerFilter
+ })
+ extension.SetFilter(constant.GracefulShutdownProviderFilterKey, func()
filter.Filter {
+ return providerFilter
+ })
Review Comment:
This test mutates global process-wide state (RootConfig and the global
filter registry via extension.SetFilter) but never restores it. That can leak
into other tests and cause order-dependent failures. Add a cleanup to restore
the previous RootConfig and unregister the temporary filters.
--
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]
