optimize: make EnhancedServiceLoader more readable #4622

lingxiao-wu · 2022-05-20T16:29:00Z

I have registered the PR changes.

Ⅰ. Describe what this PR did

refactor EnhancedServiceLoader.java to make codes more readable

Ⅱ. Does this pull request fix one issue?

no

Ⅲ. Why don't you add test cases (unit test/integration test)?

the test cases EnhancedServiceLoaderTest.java already exists

Ⅳ. Describe how to verify it

Just run the test cases EnhancedServiceLoaderTest.java to verify it

Ⅴ. Special notes for reviews

…able

funky-eyes

我认为这不算代码的重构,只是去除了部分代码警告,而增加了ClassCastException 的catch增加了应用启动时异常被吞,导致运行在错误环境下,这应该直接抛出而不是捕获,将问题阻挡在最前沿

…able

lingxiao-wu · 2022-05-21T01:22:04Z

关于此次pr我主要基于以下几点考虑：
1.SERVICE_LOADERS放在InnerEnhancedServiceLoader中维护不合理，容易与InnerEnhancedServiceLoader中的其他变量混淆，并且对于SERVICE_LOADERS的访问也都是由EnhancedServiceLoader完成的，所以放置在EnhancedServiceLoader维护更加合理。
2.将SERVICE_LOADERS移出InnerEnhancedServiceLoader后，InnerEnhancedServiceLoader中的变量都和泛型S直接相关，修改后泛型S将贯穿整个SPI加载过程，代码阅读起来更容易理解。
3.关于上述ClassCastException处理，我已经在最新的一次提交中采用了一些方法规避。

funky-eyes · 2022-05-21T16:41:46Z

我认为这算是个代码优化,而非重构,你认为呢?

关于此次pr我主要基于以下几点考虑： 1.SERVICE_LOADERS放在InnerEnhancedServiceLoader中维护不合理，容易与InnerEnhancedServiceLoader中的其他变量混淆，并且对于SERVICE_LOADERS的访问也都是由EnhancedServiceLoader完成的，所以放置在EnhancedServiceLoader维护更加合理。 2.将SERVICE_LOADERS移出InnerEnhancedServiceLoader后，InnerEnhancedServiceLoader中的变量都和泛型S直接相关，修改后泛型S将贯穿整个SPI加载过程，代码阅读起来更容易理解。 3.关于上述ClassCastException处理，我已经在最新的一次提交中采用了一些方法规避。

funky-eyes

/home/runner/work/seata/seata/common/src/main/java/io/seata/common/loader/EnhancedServiceLoader.java:24:8: Unused import - java.util.Collections. [UnusedImports]

lingxiao-wu · 2022-05-22T05:20:08Z

/home/runner/work/seata/seata/common/src/main/java/io/seata/common/loader/EnhancedServiceLoader.java:24:8: Unused import - java.util.Collections. [UnusedImports]

I've removed it

codecov-commenter · 2022-05-22T08:36:10Z

Codecov Report

Merging #4622 (913f0a5) into develop (09832bd) will increase coverage by 0.13%.
The diff coverage is 76.47%.

❗ Current head 913f0a5 differs from pull request most recent head 9bd905f. Consider uploading reports for the commit 9bd905f to get more accurate results

@@              Coverage Diff              @@
##             develop    #4622      +/-   ##
=============================================
+ Coverage      48.90%   49.04%   +0.13%     
- Complexity      4042     4058      +16     
=============================================
  Files            734      734              
  Lines          25577    25579       +2     
  Branches        3156     3157       +1     
=============================================
+ Hits           12509    12545      +36     
+ Misses         11732    11699      -33     
+ Partials        1336     1335       -1

Impacted Files	Coverage Δ
...va/io/seata/common/loader/ExtensionDefinition.java	`38.70% <50.00%> (ø)`
.../io/seata/common/loader/EnhancedServiceLoader.java	`56.48% <78.12%> (-0.53%)`	⬇️
...o/seata/server/session/AbstractSessionManager.java	`58.20% <0.00%> (-2.99%)`	⬇️
...torage/file/store/FileTransactionStoreManager.java	`56.77% <0.00%> (-2.91%)`	⬇️
...erver/storage/file/session/FileSessionManager.java	`48.40% <0.00%> (+0.63%)`	⬆️
...rage/redis/store/RedisTransactionStoreManager.java	`67.63% <0.00%> (+3.75%)`	⬆️
...java/io/seata/server/storage/SessionConverter.java	`89.09% <0.00%> (+9.09%)`	⬆️
...va/io/seata/server/console/vo/GlobalSessionVO.java	`55.88% <0.00%> (+33.82%)`	⬆️

funky-eyes · 2022-05-22T12:40:00Z

common/src/main/java/io/seata/common/loader/EnhancedServiceLoader.java

            throws ClassNotFoundException {
            //Check whether the definition has been loaded
            if (!isDefinitionContainsClazz(className, loader)) {
                Class<?> clazz = Class.forName(className, true, loader);
+                if (!type.isAssignableFrom(clazz)) {


应该抛出异常来处理,而不是返回个null

我考虑了一下，能运行到此处的异常情况有一种是SPI中指定的实现类并没有实现SPI文件指定的接口，此处应该抛出ClassCastException，并且在日志中提醒用户配置有问题。我已经在最新提交中处理，并且添加上诉场景的单元测试。
当SPI中实现类指定有问题时，日志将输出如下内容

[main] ERROR io.seata.common.loader.EnhancedServiceLoader$InnerEnhancedServiceLoader - can't cast io.seata.common.loader.FrenchHello to io.seata.common.loader.Hello1 [main] ERROR io.seata.common.loader.EnhancedServiceLoader$InnerEnhancedServiceLoader - Load [io.seata.common.loader.FrenchHello] class fail, please make sure the extension config in META-INF/seata/io.seata.common.loader.Hello1 implements io.seata.common.loader.Hello1.

…the implementation class specified therein

funky-eyes

LGTM.
Pls add changes.

https://github.com/seata/seata/blob/develop/changes/en-us/develop.md
https://github.com/seata/seata/blob/develop/changes/zh-cn/develop.md

slievrly

LGTM

refactor: refactor EnhancedServiceLoader.java to make codes more read…

566a915

…able

funky-eyes added the Do Not Merge Do not merge into develop label May 20, 2022

funky-eyes reviewed May 20, 2022

View reviewed changes

lingxiao-wu requested a review from funky-eyes May 20, 2022 16:48

refactor: refactor EnhancedServiceLoader.java to make codes more read…

ceac773

…able

funky-eyes added first-time contributor first-time contributor module/common common module and removed Do Not Merge Do not merge into develop labels May 21, 2022

funky-eyes reviewed May 21, 2022

View reviewed changes

remove UnusedImports in EnhancedServiceLoader.java

913f0a5

funky-eyes changed the title ~~refactor: refactor EnhancedServiceLoader.java to make codes more readable~~ optimize: EnhancedServiceLoader.java to make codes more readable May 22, 2022

funky-eyes added the type: optimize label May 22, 2022

funky-eyes reviewed May 22, 2022

View reviewed changes

throw ClassCastException when the SPI file name is inconsistent with …

9bd905f

…the implementation class specified therein

funky-eyes approved these changes Jun 12, 2022

View reviewed changes

funky-eyes added this to the 1.5.2 milestone Jun 12, 2022

lingxiao-wu and others added 2 commits June 29, 2022 00:00

Merge branch 'develop' into refactor-enhancedServiceLoader

6cec1cb

Merge branch 'develop' into refactor-enhancedServiceLoader

eb61811

slievrly approved these changes Jul 6, 2022

View reviewed changes

Merge branch 'develop' into refactor-enhancedServiceLoader

55ec8ff

slievrly changed the title ~~optimize: EnhancedServiceLoader.java to make codes more readable~~ optimize: make EnhancedServiceLoader more readable Jul 8, 2022

slievrly merged commit 41382b4 into apache:develop Jul 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

optimize: make EnhancedServiceLoader more readable #4622

optimize: make EnhancedServiceLoader more readable #4622

lingxiao-wu commented May 20, 2022

funky-eyes left a comment

lingxiao-wu commented May 21, 2022

funky-eyes commented May 21, 2022

funky-eyes left a comment

lingxiao-wu commented May 22, 2022

codecov-commenter commented May 22, 2022 •

edited

funky-eyes May 22, 2022

lingxiao-wu May 22, 2022

funky-eyes left a comment

slievrly left a comment

optimize: make EnhancedServiceLoader more readable #4622

optimize: make EnhancedServiceLoader more readable #4622

Conversation

lingxiao-wu commented May 20, 2022

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)?

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

funky-eyes left a comment

Choose a reason for hiding this comment

lingxiao-wu commented May 21, 2022

funky-eyes commented May 21, 2022

funky-eyes left a comment

Choose a reason for hiding this comment

lingxiao-wu commented May 22, 2022

codecov-commenter commented May 22, 2022 • edited

Codecov Report

funky-eyes May 22, 2022

Choose a reason for hiding this comment

lingxiao-wu May 22, 2022

Choose a reason for hiding this comment

funky-eyes left a comment

Choose a reason for hiding this comment

slievrly left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 22, 2022 •

edited