refactor(filter): replace config package for hystrix#3193
refactor(filter): replace config package for hystrix#3193zbchi wants to merge 1 commit intoapache:developfrom
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3193 +/- ##
===========================================
+ Coverage 46.76% 48.41% +1.65%
===========================================
Files 295 461 +166
Lines 17172 33369 +16197
===========================================
+ Hits 8031 16157 +8126
- Misses 8287 15902 +7615
- Partials 854 1310 +456 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
hystrix 已经七八年没人维护了，可以不要了吧？ |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Hystrix filter to remove its dependency on the config package and align with the Sentinel filter's API design. Users now configure Hystrix commands directly using the hystrix-go API instead of through YAML configuration.
Changes:
- Removed config package dependency and all YAML-based configuration code
- Simplified filter implementation to use hystrix-go API directly
- Changed resource name format from URL.Key() based to a colon-separated format matching Sentinel filter
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| filter/hystrix/filter.go | Removed config loading logic, error whitelist support, and synchronization primitives; simplified Invoke method to use hystrix.Do directly; added resource name generation matching Sentinel filter format |
| filter/hystrix/filter_test.go | Replaced comprehensive config-based tests with simpler API-based tests demonstrating new configuration approach |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Invoke is an implementation of filter, provides Hystrix pattern latency and fault tolerance | ||
| func (f *Filter) Invoke(ctx context.Context, invoker base.Invoker, invocation base.Invocation) result.Result { | ||
| cmdName := fmt.Sprintf("%s&method=%s", invoker.GetURL().Key(), invocation.MethodName()) | ||
| cmdName := getResourceName(invoker, invocation, f.COrP) |
There was a problem hiding this comment.
The resource name format has changed from the old implementation. Previously, it used URL.Key() which had a different format. The new format "dubbo:consumer:InterfaceName:group:version:Method(paramTypes)" is consistent with the Sentinel filter but represents a breaking change. Users who configured hystrix commands based on the old format will need to update their configuration. Consider documenting this breaking change in the PR description or migration guide.
| err := hystrix.Do(cmdName, func() error { | ||
| res = invoker.Invoke(ctx, invocation) | ||
| err := res.Error() | ||
| if err != nil { | ||
| res.SetError(NewHystrixFilterError(err, false)) | ||
| for _, reg := range f.res[invocation.MethodName()] { | ||
| if reg.MatchString(err.Error()) { | ||
| logger.Debugf("[Hystrix Filter]Error in invocation but omitted in circuit breaker: %v; %s", err, cmdName) | ||
| return nil | ||
| } | ||
| } | ||
| } | ||
| return err | ||
| return res.Error() |
There was a problem hiding this comment.
The error whitelist feature has been removed. Previously, users could configure error patterns to exclude from circuit breaker metrics. While this simplifies the implementation and aligns with the Sentinel filter approach, it's a breaking change for users who relied on this functionality. Consider documenting this removal and providing guidance on alternative approaches, such as implementing custom error filtering logic before the hystrix filter.
| func TestHystrixFilter_Invoke(t *testing.T) { | ||
| // Configure hystrix command for testing | ||
| // Resource name format: dubbo:consumer:InterfaceName:group:version:Method(paramTypes) | ||
| cmdName := "dubbo:consumer:com.ikurento.user.UserProvider:::TestMethod()" | ||
| hystrix.ConfigureCommand(cmdName, hystrix.CommandConfig{ | ||
| Timeout: 1000, | ||
| MaxConcurrentRequests: 10, | ||
| RequestVolumeThreshold: 5, | ||
| SleepWindow: 1000, | ||
| ErrorPercentThreshold: 50, | ||
| }) | ||
|
|
||
| func TestHystrixFilterInvokeFail(t *testing.T) { | ||
| hf := &Filter{} | ||
| testUrl, err := common.NewURL( | ||
| fmt.Sprintf("dubbo://%s:%d/com.ikurento.user.UserProvider", constant.LocalHostValue, constant.DefaultPort)) | ||
| url, err := common.NewURL("dubbo://127.0.0.1:20000/com.ikurento.user.UserProvider") | ||
| require.NoError(t, err) | ||
| testInvoker := testMockFailInvoker{*base.NewBaseInvoker(testUrl)} | ||
| invokeResult := hf.Invoke(context.Background(), &testInvoker, &invocation.RPCInvocation{}) | ||
| assert.NotNil(t, invokeResult) | ||
| require.Error(t, invokeResult.Error()) | ||
| } | ||
|
|
||
| func TestHystrixFilterInvokeCircuitBreak(t *testing.T) { | ||
| mockInitHystrixConfig() | ||
| hystrix.Flush() | ||
| hf := &Filter{COrP: true} | ||
| resChan := make(chan result.Result, 50) | ||
| configLoadMutex.Lock() | ||
| defer configLoadMutex.Unlock() | ||
| for i := 0; i < 50; i++ { | ||
| go func() { | ||
| testUrl, err := common.NewURL( | ||
| fmt.Sprintf("dubbo://%s:%d/com.ikurento.user.UserProvider", constant.LocalHostValue, constant.DefaultPort)) | ||
| assert.NoError(t, err) | ||
| testInvoker := testMockSuccessInvoker{*base.NewBaseInvoker(testUrl)} | ||
| invokeResult := hf.Invoke(context.Background(), &testInvoker, &invocation.RPCInvocation{}) | ||
| resChan <- invokeResult | ||
| }() | ||
| } | ||
| // This can not always pass the test when on travis due to concurrency, you can uncomment the code below and test it locally | ||
| mockInvoker := base.NewBaseInvoker(url) | ||
|
|
||
| //var lastRest bool | ||
| //for i := 0; i < 50; i++ { | ||
| // lastRest = (<-resChan).Error().(*FilterError).FailByHystrix() | ||
| //} | ||
| //Normally the last result should be true, which means the circuit has been opened | ||
| // | ||
| //assert.True(t, lastRest) | ||
| } | ||
|
|
||
| func TestHystrixFilterInvokeCircuitBreakOmitException(t *testing.T) { | ||
| mockInitHystrixConfig() | ||
| hystrix.Flush() | ||
| reg, _ := regexp.Compile(".*exception.*") | ||
| regs := []*regexp.Regexp{reg} | ||
| hf := &Filter{res: map[string][]*regexp.Regexp{"": regs}, COrP: true} | ||
| resChan := make(chan result.Result, 50) | ||
| configLoadMutex.Lock() | ||
| defer configLoadMutex.Unlock() | ||
| for i := 0; i < 50; i++ { | ||
| go func() { | ||
| testUrl, err := common.NewURL( | ||
| fmt.Sprintf("dubbo://%s:%d/com.ikurento.user.UserProvider", constant.LocalHostValue, constant.DefaultPort)) | ||
| assert.NoError(t, err) | ||
| testInvoker := testMockSuccessInvoker{*base.NewBaseInvoker(testUrl)} | ||
| invokeResult := hf.Invoke(context.Background(), &testInvoker, &invocation.RPCInvocation{}) | ||
| resChan <- invokeResult | ||
| }() | ||
| } | ||
| // This can not always pass the test when on travis due to concurrency, you can uncomment the code below and test it locally | ||
| filter := &Filter{COrP: true} | ||
| mockInvocation := invocation.NewRPCInvocation("TestMethod", []any{"OK"}, make(map[string]any)) | ||
|
|
||
| //time.Sleep(time.Second * 6) | ||
| //var lastRest bool | ||
| //for i := 0; i < 50; i++ { | ||
| // lastRest = (<-resChan).Error().(*FilterError).FailByHystrix() | ||
| //} | ||
| // | ||
| //assert.False(t, lastRest) | ||
| ctx := context.Background() | ||
| result := filter.Invoke(ctx, mockInvoker, mockInvocation) | ||
| assert.NotNil(t, result) | ||
| assert.NoError(t, result.Error()) | ||
| } |
There was a problem hiding this comment.
The test coverage is minimal compared to the old implementation. The removed tests covered important scenarios like circuit breaking behavior, error handling, and concurrent requests. Consider adding tests for: 1) invocation errors being properly handled, 2) circuit breaker opening after threshold is reached, 3) fallback behavior when circuit is open. The Sentinel filter has similar comprehensive tests (filter/sentinel/filter_test.go:45-92, 118-160) that could serve as a reference.
| // // Resource name format: dubbo:consumer:InterfaceName:group:version:Method(param1,param2) | ||
| // // Example: dubbo:consumer:com.example.GreetService:::Greet(string,string) | ||
| // hystrix.ConfigureCommand("dubbo:consumer:com.example.GreetService:::Greet(string,string)", hystrix.CommandConfig{ | ||
| // Timeout: 1000, | ||
| // MaxConcurrentRequests: 20, | ||
| // RequestVolumeThreshold: 20, | ||
| // SleepWindow: 5000, | ||
| // ErrorPercentThreshold: 50, | ||
| // }) |
There was a problem hiding this comment.
The documentation example shows parameter types as "string,string" but the test shows empty parameter types with "()". The documentation should clarify that parameter types are included only when they exist, and show a clearer example. For instance, the test uses "TestMethod()" with no parameters, which doesn't match the documentation example format.
@AlexStocks @zbchi 可以把这个filter放到 https://github.com/apache/dubbo-go-contrib 里面，正好以这个filter为例子把dubbo-go-contrib利用起来 |
用这个库 https://github.com/apache/dubbo-go-extensions 吧，https://github.com/apache/dubbo-go-contrib 这个就算了 |
Description
#2741
This PR refactors the Hystrix filter to simplify its configuration approach by removing the config package.
Users now configure Hystrix commands using the hystrix-go API directly,aligning with the Sentinel filter's API.
before
after
benifits
no longer depend on config package
direct api usage, aligns with Sentinel filter configuration approach
Checklist
develop