-
Notifications
You must be signed in to change notification settings - Fork 8.8k
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 TCC model to fescar, resolve conflicks (#582) #583
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #583 +/- ##
=============================================
+ Coverage 31.79% 31.85% +0.06%
- Complexity 798 863 +65
=============================================
Files 204 227 +23
Lines 8180 8817 +637
Branches 962 1060 +98
=============================================
+ Hits 2601 2809 +208
- Misses 5318 5695 +377
- Partials 261 313 +52
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- 统一字段对齐风格,如BusinessActionContext 与其他对其风格不一致
- TccResourceManager类名Tcc为小写,与其他不一致
- Protocols 是否考虑使用常量定义而非枚举定义?这样以便于其他其他人员能在不修改fescar-tcc的基础上扩展实现
- LocalTCCRemotingParser的getServiceDesc是否永远返回null,后续代码不起作用?
- Tcc的Annotation放在fescar-spring里是否更好?
- TC的Cacnel操作发起有可能先于实际业务的try的执行
LocalTCCRemotingParser会判断类是否有LocalTcc注解,不会永远为null, |
“统一字段对齐风格,如BusinessActionContext 与其他对其风格不一致”,“TccResourceManager类名Tcc为小写,与其他不一致” ,已经改了; “ Protocols 是否考虑使用常量定义而非枚举定义?这样以便于其他其他人员能在不修改fescar-tcc的基础上扩展实现” ,这个见仁见智吧,目前能枚举出来的RPC框架可能不会超过10个吧,我可以现在就把你能枚举出来的RPC框架加进去,虽然现在不一定会全部去支持,但这个枚举类被动的频率非常低;另外,为了保障和我们商业版的兼容性,这个枚举也不打算动; “ LocalTCCRemotingParser的getServiceDesc是否永远返回null,后续代码不起作用?”,LocalTCCRemotingParser.getServiceDesc 不是永远返回null的,你可以跑一下TCC的 localtcc demo验证一下; “Tcc的Annotation放在fescar-spring里是否更好?”, 现在是fescar-spring依赖 fescar-tcc,把TCC的枚举移到 fescar-spring中的话,fescar-tcc里面就访问不到这些TCC的注解了;所以TCC的注解还是会放在fescar-tcc里面;不过后续fescar的代码库结构可以再调整一下,现在缺一个“fescar-api ”包来存储所有api性质的类 ; “TC的Cacnel操作发起有可能先于实际业务的try的执行”,确实有这个问题,这是TCC的另外一个功能解决的问题,这次没开出来; |
DefaultRemotingParser#parserRemotingServiceInfo 在被调用之前,会先调用 DefaultRemotingParser.get().isRemoting(bean, beanName)判断一下的,如果DefaultRemotingParser.get().isRemoting(bean, beanName)为true是不会发生你说的问题,如果为false也不会进入这段代码;所以你说的问题不存在; 不过我可以再在 DefaultRemotingParser#parserRemotingServiceInfo 做一次判断兜底。 |
是的问题,不会出现,如果兜底判断了,可以把这段代码合并,因为后面的代码也会调用isRemoting |
“ Protocols 是否考虑使用常量定义而非枚举定义?这样以便于其他其他人员能在不修改fescar-tcc的基础上扩展实现”
“Tcc的Annotation放在fescar-spring里是否更好?”,
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just reviewed a little. 2 suggestions given you is here:
- Do comment in English and comment interface clearly.
- Format you code style with plugin?
* @param <S> | ||
* @return | ||
*/ | ||
public static <S> List<S> loadAll(Class<S> service){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static <S> List<S> loadAll(Class<S> service) {
@@ -100,6 +102,28 @@ | |||
return loadFile(service, activateName, loader); | |||
} | |||
|
|||
/** | |||
* 返回所有实现类实例 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment this in English.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jovany-wang I will change it to English, but there were many Chinese comments in this file before.
* @return | ||
*/ | ||
public static <S> List<S> loadAll(Class<S> service){ | ||
List<S> allInstances = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
List<S> allInstances = new ArrayList<>();
List<Class> allClazzs = getAllExtensionClass(service);
if(!CollectionUtils.isEmpty(allClazzs)){
try {
for(Class clazz : allClazzs){
allInstances.add(initInstance(service, clazz));
}
} catch (Throwable t) {
throw new EnhancedServiceNotFoundException(t);
}
}
return allInstances;
"Format you code style with plugin?" , This is already planned, I will use maven plugin to reformat all projects with p3c codestyle in next PR, There is a miss of automatic reformat mechanism now. |
@skyesx 我在后面的PR中,重新对工程结构进行重排,现在缺一个类似“fescar-api”的包放所有api,把Annotation放在 fescar-spring也不是一个最优方式 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BusinessActionContextParameter#isShardingParam
how it work ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i have some questions about actionName(TCCResource)
if(twoPhaseBusinessAction != null){ | ||
//一阶段方法, 提取TCC 服务 信息,注册TCC资源 | ||
TCCResource tccResource = new TCCResource(); | ||
tccResource.setActionName(twoPhaseBusinessAction.name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actionName的设定需要唯一,由于依赖的dubbo的jar可能来自不同团队,极有可能冲突,我们是不是可以把这个类名也加进去标记唯一 例如:com.alibaba.fescar.samples.tcc.dubbo.action.TccActionOne#+twoPhaseBusinessAction.name() 来标记整个类唯一即可。
另外对于actionName的唯一设置,我们是不是该加一个校验?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actionName的设定需要唯一,由于依赖的dubbo的jar可能来自不同团队,极有可能冲突,我们是不是可以把这个类名也加进去标记唯一 例如:com.alibaba.fescar.samples.tcc.dubbo.action.TccActionOne#+twoPhaseBusinessAction.name() 来标记整个类唯一即可。
另外对于actionName的唯一设置,我们是不是该加一个校验?
确实会有actionName冲突的可能,但是在开源版里面我们没把功能做的很强;
在我们商业版中,会把 interface和version 都拼接到 actionName中,这样才能尽量保证唯一;
你说的只拼接interface是不够的,以dubbo为例,还会存在interface相同但是version不同的情况;
我们商业版在服务端也对资源进行了冲突的校验,但是TCC开源版第一个版只打算先把最基本的功能开出去,不打算一次性把所有功能全部开出去。
import com.alibaba.fescar.core.exception.TransactionExceptionCode; | ||
import com.alibaba.fescar.core.model.BranchStatus; | ||
import com.alibaba.fescar.core.model.BranchType; | ||
import com.alibaba.fescar.core.model.Resource; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to remove unnecessary import class
目前,当前开源的版本功能比较简单,这个属性是分库分表时才会用到的,当前版本还没用到; |
}else { | ||
result = (boolean) ret; | ||
} | ||
return result ? BranchStatus.PhaseTwo_Committed:BranchStatus.PhaseTwo_CommitFailed_Retryable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when result type is TwoPhaseResult and ret.isSuccess()=false , just return PhaseTwo_CommitFailed_Retryable, why not print TwoPhaseResult#msg to check problem?
//init all resource managers | ||
List<RemotingParser> remotingParsers = EnhancedServiceLoader.loadAll(RemotingParser.class); | ||
if(CollectionUtils.isNotEmpty(remotingParsers)){ | ||
for(RemotingParser rp : remotingParsers){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addAll or allRemotingParsers= remotingParsers?
* @return boolean boolean | ||
* @throws FrameworkException the framework exception | ||
*/ | ||
public boolean isRemoting(Object bean, String beanName) throws FrameworkException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove all interface method public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@slievrly interface 中的方法默认是public的,加public和不加没什么区别;我们这边没听说一定要把interface中方法的public去掉的的要求,阿里集团那边对这个有明确要求吗 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not Alibaba's employee, @slievrly is.
* @param beanName the bean name | ||
* @return boolean boolean | ||
*/ | ||
protected boolean isTccAutoProxy(Object bean, String beanName){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can move to other class(eg:TCCUtils#isTccAutoProxy(Object bean, String beanName,ApplicationContext applicationContext))
on the issue of annotation, i think we should get agreement as soon as possible. because it's user interface , if we not change now,with later refactor, user code have to change too. 对于注解的位置问题,我觉得应该尽快统一思路,因为这是用户接口,若现在不改,后续要重构的话,用户代码也需要进行直接的变动 |
//set the parameter whose type is BusinessActionContext | ||
Class<?>[] types = method.getParameterTypes(); | ||
int argIndex = 0; | ||
for (Class<?> cls : types) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need BIZ specifies the parameter location instead of placing it at the end?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BusinessActionContext param
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why need BIZ specifies the parameter location instead of placing it at the end?
@slievrly We don't specify the location of BusinessActionContext , user can put it anywhere.
I agree with this point, the annotation is an extension built on the api. |
<dependency> | ||
<groupId>junit</groupId> | ||
<artifactId>junit</artifactId> | ||
<version>${junit.version}</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's Okey to me.
Integrated the TCC mode into seata.