Alanxtl commented on code in PR #3386:
URL: https://github.com/apache/dubbo-go/pull/3386#discussion_r3377616691
##########
filter/graceful_shutdown/provider_filter_test.go:
##########
@@ -88,6 +89,15 @@ func
TestProviderFilterOnResponseDoesNotDecrementRejectedRequest(t *testing.T) {
assert.Equal(t, int32(0), opt.Shutdown.ProviderActiveCount.Load())
}
+func TestProviderFilterAcceptsConfigPackageShutdownConfigAlias(t *testing.T) {
+ providerFilter := &providerGracefulShutdownFilter{}
+ shutdownConfig := config.NewShutDownConfigBuilder().Build()
+
+ providerFilter.Set(constant.GracefulShutdownFilterShutdownConfig,
shutdownConfig)
+
+ assert.Same(t, shutdownConfig, providerFilter.shutdownConfig)
+}
Review Comment:
ditto
##########
filter/graceful_shutdown/consumer_filter_test.go:
##########
@@ -86,6 +87,15 @@ func TestConsumerFilterInvokeWithGlobalPackage(t *testing.T)
{
assert.NoError(t, result.Error())
}
+func TestConsumerFilterAcceptsConfigPackageShutdownConfigAlias(t *testing.T) {
+ filter := &consumerGracefulShutdownFilter{}
+ shutdownConfig := config.NewShutDownConfigBuilder().Build()
+
+ filter.Set(constant.GracefulShutdownFilterShutdownConfig,
shutdownConfig)
+
+ assert.Same(t, shutdownConfig, filter.shutdownConfig)
+}
Review Comment:
这个不用加了 不要加入config的依赖
##########
global/shutdown_config.go:
##########
@@ -17,12 +17,30 @@
package global
+import (
+ "time"
+)
+
import (
"github.com/creasty/defaults"
+ "github.com/dubbogo/gost/log/logger"
+
"go.uber.org/atomic"
)
+import (
+ "dubbo.apache.org/dubbo-go/v3/common/constant"
+)
+
+const (
+ defaultShutdownTimeout = 60 * time.Second
+ defaultShutdownStepTimeout = 3 * time.Second
+ defaultShutdownNotifyTimeout = 5 * time.Second
+ defaultShutdownConsumerUpdateWaitTime = 3 * time.Second
+ defaultShutdownOfflineRequestWindowTimeout = 3 * time.Second
+)
Review Comment:
what about move these to common/const
##########
compat.go:
##########
@@ -471,19 +467,7 @@ func compatShutdownConfig(c *global.ShutdownConfig)
*config.ShutdownConfig {
if c == nil {
return nil
}
- cfg := &config.ShutdownConfig{
- Timeout: c.Timeout,
- StepTimeout: c.StepTimeout,
- NotifyTimeout: c.NotifyTimeout,
- ConsumerUpdateWaitTime: c.ConsumerUpdateWaitTime,
- RejectRequestHandler: c.RejectRequestHandler,
- InternalSignal: c.InternalSignal,
- OfflineRequestWindowTimeout: c.OfflineRequestWindowTimeout,
- RejectRequest: atomic.Bool{},
- }
- cfg.RejectRequest.Store(c.RejectRequest.Load())
-
- return cfg
Review Comment:
这个不用删 到时候我们直接把compat这个文件删掉
##########
config/graceful_shutdown_config.go:
##########
@@ -17,141 +17,18 @@
package config
-import (
- "time"
-)
-
import (
"github.com/creasty/defaults"
-
- "github.com/dubbogo/gost/log/logger"
-
- "go.uber.org/atomic"
)
import (
- "dubbo.apache.org/dubbo-go/v3/common/constant"
-)
-
-const (
- defaultTimeout = 60 * time.Second
- defaultStepTimeout = 3 * time.Second
- defaultNotifyTimeout = 5 * time.Second
- defaultConsumerUpdateWaitTime = 3 * time.Second
- defaultOfflineRequestWindowTimeout = 3 * time.Second
+ "dubbo.apache.org/dubbo-go/v3/global"
)
-// ShutdownConfig is used as configuration for graceful shutdown
-type ShutdownConfig struct {
- /*
- * Total timeout. Even though we don't release all resources,
- * the applicationConfig will shutdown if the costing time is over this
configuration. The unit is ms.
- * default value is 60 * 1000 ms = 1 minutes
- * In general, it should be bigger than 3 * StepTimeout.
- */
- Timeout string `default:"60s" yaml:"timeout" json:"timeout,omitempty"
property:"timeout"`
- /*
- * the timeout on each step. You should evaluate the response time of
request
- * and the time that client noticed that server shutdown.
- * For example, if your client will received the notification within
10s when you start to close server,
- * and the 99.9% requests will return response in 2s, so the
StepTimeout will be bigger than(10+2) * 1000ms,
- * maybe (10 + 2*3) * 1000ms is a good choice.
- */
- StepTimeout string `default:"3s" yaml:"step-timeout"
json:"step.timeout,omitempty" property:"step.timeout"`
-
- /*
- * NotifyTimeout means the timeout budget for actively notifying
long-connection consumers
- * during graceful shutdown. It only controls the notify step and
should not be coupled to
- * request draining timeouts.
- */
- NotifyTimeout string `default:"5s" yaml:"notify-timeout"
json:"notify.timeout,omitempty" property:"notify.timeout"`
-
- /*
- * ConsumerUpdateWaitTime means when provider is shutting down, after
the unregister, time to wait for client to
- * update invokers. During this time, incoming invocation can be
treated normally.
- */
- ConsumerUpdateWaitTime string `default:"3s"
yaml:"consumer-update-wait-time" json:"consumerUpdate.waitTIme,omitempty"
property:"consumerUpdate.waitTIme"`
- // when we try to shutdown the applicationConfig, we will reject the
new requests. In most cases, you don't need to configure this.
- RejectRequestHandler string `yaml:"reject-handler"
json:"reject-handler,omitempty" property:"reject_handler"`
- // internal listen kill signal，the default is true.
- InternalSignal *bool `default:"true" yaml:"internal-signal"
json:"internal.signal,omitempty" property:"internal.signal"`
- // offline request window length
- OfflineRequestWindowTimeout string `default:"3s"
yaml:"offline-request-window-timeout"
json:"offlineRequestWindowTimeout,omitempty"
property:"offlineRequestWindowTimeout"`
- // true -> new request will be rejected.
- RejectRequest atomic.Bool
- // active invocation
- ConsumerActiveCount atomic.Int32
- ProviderActiveCount atomic.Int32
-
- // provider last received request timestamp
- ProviderLastReceivedRequestTime atomic.Time
-}
-
-// Prefix dubbo.shutdown
-func (config *ShutdownConfig) Prefix() string {
- return constant.ShutdownConfigPrefix
-}
-
-func (config *ShutdownConfig) GetTimeout() time.Duration {
- result, err := time.ParseDuration(config.Timeout)
- if err != nil {
- logger.Errorf("The Timeout configuration is invalid: %s, and we
will use the default value: %s, err: %v",
- config.Timeout, defaultTimeout.String(), err)
- return defaultTimeout
- }
- return result
-}
-
-func (config *ShutdownConfig) GetStepTimeout() time.Duration {
- result, err := time.ParseDuration(config.StepTimeout)
- if err != nil {
- logger.Errorf("The StepTimeout configuration is invalid: %s,
and we will use the default value: %s, err: %v",
- config.StepTimeout, defaultStepTimeout.String(), err)
- return defaultStepTimeout
- }
- return result
-}
-
-func (config *ShutdownConfig) GetNotifyTimeout() time.Duration {
- result, err := time.ParseDuration(config.NotifyTimeout)
- if err != nil {
- logger.Errorf("The NotifyTimeout configuration is invalid: %s,
and we will use the default value: %s, err: %v",
- config.NotifyTimeout, defaultNotifyTimeout.String(),
err)
- return defaultNotifyTimeout
- }
- return result
-}
-
-func (config *ShutdownConfig) GetOfflineRequestWindowTimeout() time.Duration {
- result, err := time.ParseDuration(config.OfflineRequestWindowTimeout)
- if err != nil {
- logger.Errorf("The OfflineRequestWindowTimeout configuration is
invalid: %s, and we will use the default value: %s, err: %v",
- config.OfflineRequestWindowTimeout,
defaultOfflineRequestWindowTimeout.String(), err)
- return defaultOfflineRequestWindowTimeout
- }
- return result
-}
-
-func (config *ShutdownConfig) GetConsumerUpdateWaitTime() time.Duration {
- result, err := time.ParseDuration(config.ConsumerUpdateWaitTime)
- if err != nil {
- logger.Errorf("The ConsumerUpdateTimeout configuration is
invalid: %s, and we will use the default value: %s, err: %v",
- config.ConsumerActiveCount.Load(),
defaultConsumerUpdateWaitTime.String(), err)
- return defaultConsumerUpdateWaitTime
- }
- return result
-}
-
-func (config *ShutdownConfig) GetInternalSignal() bool {
- if config.InternalSignal == nil {
- return false
- }
- return *config.InternalSignal
-}
-
-func (config *ShutdownConfig) Init() error {
- return defaults.Set(config)
-}
Review Comment:
这个不用删 到时候我们直接把config这个包删掉
--
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]
