apache
bugfix: fix the failure of @BusinessActionContextParamete annotation …
Ⅰ. Describe what this PR did fix the failure of @BusinessActionContextParamete annotation to set parameters into io.seata.rm.tcc.api.BusinessActionContext at TCC mode
Ⅱ. Does this pull request fix one issue? fixes #6454
Ⅲ. Why don't you add test cases (unit test/integration test)? Ⅳ. Describe how to verify it Ⅴ. Special notes for reviews
May I know what is going on? How can I merge my pull request to the target branch?
May I know what is going on? How can I merge my pull request to the target branch?
According to the community CONTRIBUTING.md, you may not merge PR directly. After someone who has write permission reviews and approves, and then it can be merged.
Can you shed some lights on why changing
targetBeantoinvocation.getTarget()can sort it out? The latter can be subclass of the former, is that what you are tring to solve? As the picture shows, if we just want to find the annotation @TwoPhaseBusinessAction on the interfaces of the invoked TCC try method,invocation.getTarget()is good enough to handle it. Its proxytargetBeanonly increases the complexity. The @TwoPhaseBusinessAction in my demo project is on the TCC try method of interface alibaba.cloud.spring.provider.service.StockService.
Can you shed some lights on why changing
targetBeantoinvocation.getTarget()can sort it out? The latter can be subclass of the former, is that what you are tring to solve? As the picture shows, if we just want to find the annotation @TwoPhaseBusinessAction on the interfaces of the invoked TCC try method,invocation.getTarget()is good enough to handle it. Its proxytargetBeanonly increases the complexity. The @TwoPhaseBusinessAction in my demo project is on the TCC try method of interface alibaba.cloud.spring.provider.service.StockService.
As shown in the figure you provided, both targetBean and invocation.getTarget() can find the deduct method. So, this PR is a complexity optimization not a bugfix for #6454, right?
That makes sense for me. What do you think @funky-eyes?
I agree with you.
Can you shed some lights on why changing
targetBeantoinvocation.getTarget()can sort it out? The latter can be subclass of the former, is that what you are tring to solve? As the picture shows, if we just want to find the annotation @TwoPhaseBusinessAction on the interfaces of the invoked TCC try method,invocation.getTarget()is good enough to handle it. Its proxytargetBeanonly increases the complexity. The @TwoPhaseBusinessAction in my demo project is on the TCC try method of interface alibaba.cloud.spring.provider.service.StockService.As shown in the figure you provided, both
targetBeanandinvocation.getTarget()can find thedeductmethod. So, this PR is a complexity optimization not a bugfix for #6454, right?
@ptyin It is really a bugfix. I do the optimization by the way. There several bugs here:
1.The RuntimeException is thrown too early. It should thrown later outside the for-loop.
2.The method initCommonFenceCleanTask may not execute in some scenarios. The early thrown RuntimeException may also lead to it. There are also other scenarios.
3. Last but most importantly, the later executing method org.apache.seata.integration.tx.api.interceptor.ActionInterceptorHandler#fetchActionRequestContext needs its parameter variable "method " with @TwoPhaseBusinessAction. In my demo project, if variable "method" stands for interface StockService annotated by @TwoPhaseBusinessAction, fetchActionRequestContext will successfully get the parameters annotated by @BusinessActionContextParameter and put them in context. If variable "method" stands for StockServiceImpl#deduct whose interface method is annotated by @TwoPhaseBusinessAction, fetchActionRequestContext will fail to get all the parameters annotated by @BusinessActionContextParameter. What I do in the bugfix is to get the exact method with @TwoPhaseBusinessAction
The third point, are you trying to express that when using @BusinessActionContextParameter in the interface and @TwoPhaseBusinessAction in the implementation class, it leads to the invalidation of @BusinessActionContextParameter?
The third point, are you trying to express that when using @BusinessActionContextParameter in the interface and @TwoPhaseBusinessAction in the implementation class, it leads to the invalidation of @BusinessActionContextParameter?
Nah. What I described in the third point is that a variable "method" that exactly stands for the TCC try method annnotated by @TwoPhaseBusinessAction and @BusinessActionContextParameter shoud be passed to org.apache.seata.integration.tx.api.interceptor.ActionInterceptorHandler#fetchActionRequestContext. Or fetchActionRequestContext will fail to get the parameters annotated by @BusinessActionContextParameter.
@TwoPhaseBusinessAction and @BusinessActionContextParameter on my demo project:
alibaba.cloud.spring.provider.service.StockService.deduct instead of alibaba.cloud.spring.provider.service.StockServiceImpl.deduct should be passed to fetchActionRequestContext.
What I did in the bugfix is to get the exact method annotated by @TwoPhaseBusinessAction and @BusinessActionContextParameter. (I suppose that it is a convention to put them together on a TCC try method of interface, at least putting them together) @ptyin @funky-eyes
What I did in the bugfix is to get the exact method annotated by @TwoPhaseBusinessAction and @BusinessActionContextParameter. (I suppose that it is a convention to put them together on a TCC try method of interface, at least putting them together) @ptyin @funky-eyes
我们未来希望注解都是在实现类中，而不是在接口上，目前的实现中我们之前就发现了子类会覆盖父类，导致最终的参数读取行为不准。由于tcc的特殊性，他的接口可能是由provider提供，如果注解都写在接口上时，会导致consumer与provider都进行了方法切面等行为，这是不可取的，经过社区的会议，未来会不允许注解在接口上，而是需要再实现类上
In the future, we prefer annotations to be placed in implementation classes rather than interfaces. In our current implementation, we have encountered issues where subclasses override parent classes, leading to inaccurate parameter reading behavior. Due to the unique nature of TCC, its interfaces may be provided by the provider. If annotations are placed on interfaces, it will result in both the consumer and the provider performing method interception, which is undesirable. Following discussions within the community, annotations will not be allowed on interfaces in the future, and instead, they will need to be placed on implementation classes.
https://github.com/apache/incubator-seata/issues/6235
What I did in the bugfix is to get the exact method annotated by @TwoPhaseBusinessAction and @BusinessActionContextParameter. (I suppose that it is a convention to put them together on a TCC try method of interface, at least putting them together) @ptyin @funky-eyes
我们未来希望注解都是在实现类中，而不是在接口上，目前的实现中我们之前就发现了子类会覆盖父类，导致最终的参数读取行为不准。由于tcc的特殊性，他的接口可能是由provider提供，如果注解都写在接口上时，会导致consumer与provider都进行了方法切面等行为，这是不可取的，经过社区的会议，未来会不允许注解在接口上，而是需要再实现类上
In the future, we prefer annotations to be placed in implementation classes rather than interfaces. In our current implementation, we have encountered issues where subclasses override parent classes, leading to inaccurate parameter reading behavior. Due to the unique nature of TCC, its interfaces may be provided by the provider. If annotations are placed on interfaces, it will result in both the consumer and the provider performing method interception, which is undesirable. Following discussions within the community, annotations will not be allowed on interfaces in the future, and instead, they will need to be placed on implementation classes.
#6235
Make sense! Thanks for your patience! Could you please help me merge this pull request? @funky-eyes @ptyin
What I did in the bugfix is to get the exact method annotated by @TwoPhaseBusinessAction and @BusinessActionContextParameter. (I suppose that it is a convention to put them together on a TCC try method of interface, at least putting them together) @ptyin @funky-eyes
我们未来希望注解都是在实现类中，而不是在接口上，目前的实现中我们之前就发现了子类会覆盖父类，导致最终的参数读取行为不准。由于tcc的特殊性，他的接口可能是由provider提供，如果注解都写在接口上时，会导致consumer与provider都进行了方法切面等行为，这是不可取的，经过社区的会议，未来会不允许注解在接口上，而是需要再实现类上 In the future, we prefer annotations to be placed in implementation classes rather than interfaces. In our current implementation, we have encountered issues where subclasses override parent classes, leading to inaccurate parameter reading behavior. Due to the unique nature of TCC, its interfaces may be provided by the provider. If annotations are placed on interfaces, it will result in both the consumer and the provider performing method interception, which is undesirable. Following discussions within the community, annotations will not be allowed on interfaces in the future, and instead, they will need to be placed on implementation classes. #6235
Make sense! Thanks for your patience! Could you please help me merge this pull request? @funky-eyes @ptyin
I will run some tests on this PR, and if the tests pass, I will merge it.
It did not fix the issue where BusinessActionContextParameter is ineffective in interfaces
I suppose it is a convetion to put @TwoPhaseBusinessAction and @BusinessActionContextParameter together. May I know where your @TwoPhaseBusinessAction is ? In my bugfix, it makes TCC mode work well in scenarios putting them together on the interface. While it will not be supported in the future.
Here is my demo project. It works well on my side. If possible, upload your code to the repository so that I can have a look.
我的例子就是上面所说的，当TwoPhaseBusinessAction在实现类中，而BusinessActionContextParameter在接口中时，会读取不到BusinessActionContextParameter相关的参数，我认为6454中描述的是我这种情况，所以无法正确读取到参数。 My example is exactly as mentioned above, where the BusinessActionContextParameter is in the interface while TwoPhaseBusinessAction is in the implementation class. In this scenario, the parameters related to BusinessActionContextParameter cannot be read correctly. I believe that what is described in 6454 is similar to my situation, hence the inability to correctly read the parameters.
我的例子就是上面所说的，当TwoPhaseBusinessAction在实现类中，而BusinessActionContextParameter在接口中时，会读取不到BusinessActionContextParameter相关的参数，我认为6454中描述的是我这种情况，所以无法正确读取到参数。 My example is exactly as mentioned above, where the BusinessActionContextParameter is in the interface while TwoPhaseBusinessAction is in the implementation class. In this scenario, the parameters related to BusinessActionContextParameter cannot be read correctly. I believe that what is described in 6454 is similar to my situation, hence the inability to correctly read the parameters.
@funky-eyes As far as I know, The author of issue 6454 has just put the two annotations together on the TCC try method, just as the picture shown below. And I consider it a little weird to put them separately. ￣□￣｜｜
https://github.com/apache/incubator-seata/issues/6235 According to 6454, it's not possible to determine whether the user has also added the TwoPhaseBusinessAction annotation in the implementation class. However, based on 6235, it can be inferred that users of 6454 may behave similarly to users of 6235, where the issue arises due to the subclass overriding the superclass, causing the annotation to be ineffective. My proposed solution is to prioritize the subclass, attempting to read the annotation from the superclass method only if the subclass method does not have the annotation. The same approach would be applied to handling parameters. This way, we can effectively resolve the issue.
#6235 According to 6454, it's not possible to determine whether the user has also added the TwoPhaseBusinessAction annotation in the implementation class. However, based on 6235, it can be inferred that users of 6454 may behave similarly to users of 6235, where the issue arises due to the subclass overriding the superclass, causing the annotation to be ineffective. My proposed solution is to prioritize the subclass, attempting to read the annotation from the superclass method only if the subclass method does not have the annotation. The same approach would be applied to handling parameters. This way, we can effectively resolve the issue.
I feel you. It is really a best practice to put the two annotations on the TCC try method of the implementation class.
Acutally the demo in the official document is as shown below. Usually the users just refer to the demo and find setting parameters to context failed in this way, just like me. o(╯□╰)o
So in a sense, this bugfix make it work well for the current seata version.
@funky-eyes I don't know what's going about this PR. What else do I need to do to merge this PR? Could you tell me? And I have a doubt: Why can you still check issues and PRs during work hours? Don’t you have to work? Or is this what you do for work? O(∩_∩)O
@funky-eyes I don't know what's going about this PR. What else do I need to do to merge this PR? Could you tell me? And I have a doubt: Why can you still check issues and PRs during work hours? Don’t you have to work? Or is this what you do for work? O(∩_∩)O
I plan to consider merging this PR in version 2.2, and I would like it to change the annotation method to a complementary form with priority setting, rather than solely using annotations from the parent class or the subclass. @slievrly @wangliang181230 @wt-better What do you guys think?