Sentinel icon indicating copy to clipboard operation
Sentinel copied to clipboard

fix #3386

Open dowenliu-xyz opened this issue 1 year ago • 1 comments

Describe what this PR does / why we need it

This PR contains fixes for #3386

Does this pull request fix one issue?

Fixes #3386

Describe how you did it

Combine origin method's parameter type canonical names into register map key.

Describe how to verify it

See bug #3386 to repreduce and verify.

Special notes for reviews

dowenliu-xyz avatar May 12 '24 16:05 dowenliu-xyz

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar May 12 '24 16:05 CLAassistant

这看起来确实是个问题,但是要不要在运行时支持这种同名我觉得可能得再讨论下,因为这样做会引入一些不必要的额外的复杂度,但其实只要不重名就不会有问题,另外 Issue 中的这个插件看起来是个不错的解决方案

LearningGp avatar May 21 '24 11:05 LearningGp

#3397 也要改这一处。我觉得改 #3397 还得把返回值类型信息也加到 key 里,复杂度会更高些 😂

dowenliu-xyz avatar May 21 '24 11:05 dowenliu-xyz

从概率论上讲,代码里出现重名导致这个bug是一定会出现的。所以要讨论的是怎么处理这个bug。 要么就这么放着不处理,相当于认为是快速失败,那么遇到这个问题的团队能不能避免生产事故就要看研发有没有了解到这个bug、测试有没有测到这个场景了;否则就进行修复。 修复方案我的想法是既要避免报错,也保证流控不失效(不打破研发设计预期)。 关于复杂度,我没觉得这是个问题,而且我变更的代码我自己觉得还挺可控的,不复杂吧。😂 @LearningGp 你有其他的修复思路吗?

P.S. 欢迎试用插件。如果觉得不错,可以留言评论+分享。插件已设置开源项目 free 🤝。

dowenliu-xyz avatar May 21 '24 12:05 dowenliu-xyz

Codecov Report

Attention: Patch coverage is 71.42857% with 12 lines in your changes are missing coverage. Please review.

Project coverage is 45.85%. Comparing base (dbd3c06) to head (945468f). Report is 1 commits behind head on 1.8.

Files Patch % Lines
...nterceptor/AbstractSentinelInterceptorSupport.java 0.00% 8 Missing :warning:
...l/annotation/aspectj/ResourceMetadataRegistry.java 84.61% 1 Missing and 1 partial :warning:
...tion/cdi/interceptor/ResourceMetadataRegistry.java 84.61% 1 Missing and 1 partial :warning:
Additional details and impacted files
@@             Coverage Diff              @@
##                1.8    #3395      +/-   ##
============================================
- Coverage     45.90%   45.85%   -0.06%     
  Complexity     2148     2148              
============================================
  Files           431      431              
  Lines         12906    12932      +26     
  Branches       1728     1728              
============================================
+ Hits           5925     5930       +5     
- Misses         6281     6301      +20     
- Partials        700      701       +1     
Components Coverage Δ
sentinel-adapter 43.22% <ø> (ø)
sentinel-cluster 23.71% <ø> (-0.08%) :arrow_down:
sentinel-core 59.64% <ø> (+0.01%) :arrow_up:
sentinel-extension 45.88% <71.42%> (-0.27%) :arrow_down:
sentinel-logging 54.54% <ø> (ø)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar May 24 '24 02:05 codecov[bot]

如果方便的话,麻烦在sentinel-demo-annotation-spring-aop模块中完善下Demo以体现新的特性

LearningGp avatar May 24 '24 02:05 LearningGp

如果方便的话,麻烦在sentinel-demo-annotation-spring-aop模块中完善下Demo以体现新的特性

OK,我这两天看时间搞下。

dowenliu-xyz avatar May 24 '24 03:05 dowenliu-xyz

Done.

Run module sentinel-demo-annotation-spring-aop, and run following requests (in curl format) to verify that fallbacks won't overlap each other. Once the module started, you can call requests in any sequence.

curl -XGET localhost:19966/bar?t=
curl -XGET localhost:19966/foo?t=-1

dowenliu-xyz avatar May 24 '24 05:05 dowenliu-xyz

另外昨天还看了下 resilience4j 的做法。 https://github.com/resilience4j/resilience4j/blob/eeaf57a8217ded7fe14bad511454f263f9e6f06d/resilience4j-spring6/src/main/java/io/github/resilience4j/spring6/fallback/FallbackMethod.java#L85 他们的 MethodMeta 结构我觉得值得参考。(可以直接照抄😅)

dowenliu-xyz avatar May 24 '24 05:05 dowenliu-xyz

另外昨天还看了下 resilience4j 的做法。 https://github.com/resilience4j/resilience4j/blob/eeaf57a8217ded7fe14bad511454f263f9e6f06d/resilience4j-spring6/src/main/java/io/github/resilience4j/spring6/fallback/FallbackMethod.java#L85 他们的 MethodMeta 结构我觉得值得参考。(可以直接照抄😅)

感谢贡献🚀,欢迎后续进行进一步优化

LearningGp avatar May 29 '24 12:05 LearningGp