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

Support setting class-level defaultFallback for annotation extension #1493

Merged
merged 5 commits into from
May 29, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@
* The annotation indicates a definition of Sentinel resource.
*
* @author Eric Zhao
* @author zhaoyuguang
* @since 0.1.1
*/
@Target(ElementType.METHOD)
@Target({ElementType.METHOD, ElementType.TYPE})
@Retention(RetentionPolicy.RUNTIME)
@Inherited
public @interface SentinelResource {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
* Some common functions for Sentinel annotation aspect.
*
* @author Eric Zhao
* @author zhaoyuguang
*/
public abstract class AbstractSentinelAspectSupport {

Expand Down Expand Up @@ -186,7 +187,13 @@ private Method extractFallbackMethod(ProceedingJoinPoint pjp, String fallbackNam
private Method extractDefaultFallbackMethod(ProceedingJoinPoint pjp, String defaultFallback,
Class<?>[] locationClass) {
if (StringUtil.isBlank(defaultFallback)) {
return null;
SentinelResource annotationClass = pjp.getTarget().getClass().getAnnotation(SentinelResource.class);
if (annotationClass != null && StringUtil.isNotBlank(annotationClass.defaultFallback())) {
defaultFallback = annotationClass.defaultFallback();
locationClass = annotationClass.fallbackClass();
Copy link
Member

Choose a reason for hiding this comment

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

The priority of method-level annotation is higher than class-level annotation, so maybe here we need to check whether original locationClass is absent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

如果 locationClass 没有配置,那么就是用当前类的作为默认,这样的逻辑合适么?

Copy link
Member

Choose a reason for hiding this comment

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

解析优先级:方法级别的 fallbackClass > 类级别的 fallbackClass,若不存在则用当前类替代。

Copy link
Collaborator Author

@zhaoyuguang zhaoyuguang May 25, 2020

Choose a reason for hiding this comment

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

假如方法注解 写了的locationClass 没写defaultFallback
而类注解写了的locationClass 和defaultFallback
在这种case下是不是 使用者配置错误 应该全部使用类注解上的locationClass 和defaultFallback比较好?

Copy link
Member

Choose a reason for hiding this comment

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

考虑一下这种场景(不一定合理):某个方法希望 override 对应的 fallbackClass,这种情况的话 可能还不能判定为配置错误

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

改了下逻辑 如果取方法注解 -> 取类注解 -> 取本类

Copy link
Collaborator

@cdfive cdfive May 26, 2020

Choose a reason for hiding this comment

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

方法级别的 fallbackClass > 类级别的 fallbackClass

看目前代码已符合满足该场景了

改了下逻辑 如果取方法注解 -> 取类注解 -> 取本类

不过这句好像并没有完全覆盖到。
比如方法级的defaultFallback不为空,但方法级的fallbackClass为空,目前没有取类级别的,取的本类。因为第一个if判断是方法级别的defaultFallback。
这个场景也不一定合理,就看是否需要完全按方法注解 -> 取类注解 -> 取本类优先级了。

BTW,有个疑问:
SentinelResource注解:
Class<?>[] blockHandlerClass() default {};
Class<?>[] fallbackClass() default {};
为何要设计成数组呢?看代码里只取了[0]第一个,是不是用Class<?>就好

Copy link
Member

Choose a reason for hiding this comment

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

方法级别的 fallbackClass > 类级别的 fallbackClass

看目前代码已符合满足该场景了

改了下逻辑 如果取方法注解 -> 取类注解 -> 取本类

不过这句好像并没有完全覆盖到。
比如方法级的defaultFallback不为空,但方法级的fallbackClass为空,目前没有取类级别的,取的本类。因为第一个if判断是方法级别的defaultFallback。
这个场景也不一定合理,就看是否需要完全按方法注解 -> 取类注解 -> 取本类优先级了。

We may improve this later.

BTW,有个疑问:
SentinelResource注解:
Class<?>[] blockHandlerClass() default {};
Class<?>[] fallbackClass() default {};
为何要设计成数组呢?看代码里只取了[0]第一个,是不是用Class<?>就好

Actually fallbackClass is optional. If using Class<?>, it cannot be null anymore.

} else {
return null;
}
}
boolean mustStatic = locationClass != null && locationClass.length >= 1;
Class<?> clazz = mustStatic ? locationClass[0] : pjp.getTarget().getClass();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package com.alibaba.csp.sentinel.annotation.aspectj.integration;

import com.alibaba.csp.sentinel.annotation.aspectj.integration.config.AopTestConfig;
import com.alibaba.csp.sentinel.annotation.aspectj.integration.service.BarService;
import com.alibaba.csp.sentinel.annotation.aspectj.integration.service.FooService;
import com.alibaba.csp.sentinel.annotation.aspectj.integration.service.FooUtil;
import com.alibaba.csp.sentinel.node.ClusterNode;
Expand Down Expand Up @@ -47,6 +48,8 @@ public class SentinelAnnotationIntegrationTest extends AbstractJUnit4SpringConte

@Autowired
private FooService fooService;
@Autowired
private BarService barService;

@Test
public void testProxySuccessful() {
Expand Down Expand Up @@ -181,6 +184,29 @@ public void testNormalBlockHandlerAndFallback() throws Exception {
assertThat(cn.blockQps()).isPositive();
}

@Test
public void testClassLevelDefaultFallbackWithSingleParam() {
assertThat(barService.anotherBar(1)).isEqualTo("Hello for 1");
String resourceName = "apiAnotherBarWithDefaultFallback";
ClusterNode cn = ClusterBuilderSlot.getClusterNode(resourceName);
assertThat(cn).isNotNull();
assertThat(cn.passQps()).isPositive();

assertThat(barService.doSomething(1)).isEqualTo("do something");
String resourceName1 = "com.alibaba.csp.sentinel.annotation.aspectj.integration.service.BarService:doSomething(int)";
ClusterNode cn1 = ClusterBuilderSlot.getClusterNode(resourceName1);
assertThat(cn1).isNotNull();
assertThat(cn1.passQps()).isPositive();

assertThat(barService.anotherBar(5758)).isEqualTo("eee...");
assertThat(cn.exceptionQps()).isPositive();
assertThat(cn.blockQps()).isZero();

assertThat(barService.doSomething(5758)).isEqualTo("GlobalFallback:doFallback");
assertThat(cn1.exceptionQps()).isPositive();
assertThat(cn1.blockQps()).isZero();
}

@Before
public void setUp() throws Exception {
FlowRuleManager.loadRules(new ArrayList<FlowRule>());
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
/*
* Copyright 1999-2018 Alibaba Group Holding Ltd.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package com.alibaba.csp.sentinel.annotation.aspectj.integration.service;

import com.alibaba.csp.sentinel.annotation.SentinelResource;
import org.springframework.stereotype.Service;

/**
* @author zhaoyuguang
*/
@Service
@SentinelResource(defaultFallback = "doFallback", fallbackClass = GlobalFallback.class)
public class BarService {

@SentinelResource(value = "apiAnotherBarWithDefaultFallback", defaultFallback = "fallbackFunc")
public String anotherBar(int i) {
if (i == 5758) {
throw new IllegalArgumentException("oops");
}
return "Hello for " + i;
}

@SentinelResource()
public String doSomething(int i) {
if (i == 5758) {
throw new IllegalArgumentException("oops");
}
return "do something";
}

public String fallbackFunc(Throwable t) {
System.out.println(t.getMessage());
return "eee...";
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package com.alibaba.csp.sentinel.annotation.aspectj.integration.service;

/**
* @author zhaoyuguang
*/

sczyh30 marked this conversation as resolved.
Show resolved Hide resolved
public class GlobalFallback {

public static String doFallback(Throwable t) {
return "GlobalFallback:doFallback";
}
}