Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add global fallback support for AspectJ annotation extension #3383

Open
wants to merge 3 commits into
base: 1.8
Choose a base branch
from

Conversation

luffy0223
Copy link

Describe what this PR does / why we need it

Add global fallback support for AspectJ annotation extension

Does this pull request fix one issue?

Fixes #3110

Describe how you did it

add global fallback interface and let it as the member of @SentinelResource,
cache and call the global fallback instance in the aspect when origin method throws Throwable

Describe how to verify it

Use unit test to verify the correctness

Special notes for reviews

NONE

Copy link

codecov bot commented Apr 24, 2024

Codecov Report

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

Project coverage is 45.96%. Comparing base (cd02b1d) to head (3fb3244).
Report is 4 commits behind head on 1.8.

Files Patch % Lines
...l/annotation/aspectj/ResourceMetadataRegistry.java 60.00% 5 Missing and 1 partial ⚠️
...a/csp/sentinel/fallback/DefaultGlobalFallback.java 0.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##                1.8    #3383      +/-   ##
============================================
+ Coverage     45.89%   45.96%   +0.07%     
- Complexity     2145     2152       +7     
============================================
  Files           431      432       +1     
  Lines         12903    12926      +23     
  Branches       1727     1728       +1     
============================================
+ Hits           5922     5942      +20     
- Misses         6279     6283       +4     
+ Partials        702      701       -1     
Components Coverage Δ
sentinel-adapter 43.22% <ø> (ø)
sentinel-cluster 23.71% <ø> (ø)
sentinel-core 59.62% <0.00%> (-0.01%) ⬇️
sentinel-extension 46.60% <72.72%> (+0.41%) ⬆️
sentinel-logging 54.54% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

* If no specific global fallback handler is provided, the default {@link DefaultGlobalFallback} class will be used.
* @return The class that serves as the global fallback handler.
*/
Class<? extends IGlobalFallback> globalFallback() default DefaultGlobalFallback.class;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the scenario the related issue(#3110) aims to solve is to have a global fallback even when neither fallback nor defaultFallback is configured in the annotations. So, the global fallback should be configured in the construction method of SentinelResourceAspect, rather than through annotations (like what defaultFallback did).

@@ -53,6 +53,25 @@ public String fooWithFallback(int i) throws Exception {
return "Hello for " + i;
}

@SentinelResource(value = "apiFooWithFallback", globalFallback = AnnotationGlobalFallback.class,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If convenient, could you please add relevant example code in the sentinel-demo-annotation-spring-aop module?

@LearningGp LearningGp added to-review To review area/annotation Issues or PRs related to annotation support labels May 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/annotation Issues or PRs related to annotation support to-review To review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add global fallback support for AspectJ annotation extension
2 participants