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

optimize: kryo 5.4.0 compatibility with jdk17 #5243

Merged
merged 11 commits into from
Jan 29, 2023

Conversation

JavaLionLi
Copy link
Contributor

@JavaLionLi JavaLionLi commented Jan 10, 2023

  • I have registered the PR changes.

Ⅰ. 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

@@ -71,7 +71,7 @@

<protobuf.version>3.11.4</protobuf.version>
<grpc.version>1.27.1</grpc.version>
<kryo.version>5.3.0</kryo.version>
<kryo.version>5.4.0</kryo.version>
Copy link
Contributor

Choose a reason for hiding this comment

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

        kryo.register(Arrays.asList("").getClass(), new ArraysAsListSerializer());
        kryo.register(GregorianCalendar.class, new GregorianCalendarSerializer());

这两段在jdk17使用到KryoSerializerFactory kryo序列化上会报错,有测试过吗?我的另一个pr里改为了
kryo.register(Collections.singletonList("").getClass());
kryo.register(GregorianCalendar.class);
后可兼容jdk17,5.4.0上老的方式,注册了指定的序列化器进去能兼容吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        kryo.register(Arrays.asList("").getClass(), new ArraysAsListSerializer());
        kryo.register(GregorianCalendar.class, new GregorianCalendarSerializer());

这两段在jdk17使用到KryoSerializerFactory kryo序列化上会报错,有测试过吗?我的另一个pr里改为了 kryo.register(Collections.singletonList("").getClass()); kryo.register(GregorianCalendar.class); 后可兼容jdk17,5.4.0上老的方式,注册了指定的序列化器进去能兼容吗?

我测试一下看看

Copy link
Contributor Author

Choose a reason for hiding this comment

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

        kryo.register(Arrays.asList("").getClass(), new ArraysAsListSerializer());
        kryo.register(GregorianCalendar.class, new GregorianCalendarSerializer());

这两段在jdk17使用到KryoSerializerFactory kryo序列化上会报错,有测试过吗?我的另一个pr里改为了 kryo.register(Collections.singletonList("").getClass()); kryo.register(GregorianCalendar.class); 后可兼容jdk17,5.4.0上老的方式,注册了指定的序列化器进去能兼容吗?

image

源码中是这样的 单测都是通过的

不过idea报了警告
image

所以我改成如下格式了
image

Copy link
Contributor Author

@JavaLionLi JavaLionLi Jan 10, 2023

Choose a reason for hiding this comment

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

image
这是个什么错呢

Copy link
Contributor

Choose a reason for hiding this comment

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

代码风格问题,看下提示的地方进行code format之类的就像了,往这个日志上面翻翻有具体提示的

Copy link
Contributor

Choose a reason for hiding this comment

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

简单点可以直接不放进去序列化器,让kryo自适应

Copy link
Contributor

Choose a reason for hiding this comment

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

顺便把上面测试jdk17的兼容性代码弄个测试用例放进去吧

Copy link
Contributor Author

Choose a reason for hiding this comment

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

顺便把上面测试jdk17的兼容性代码弄个测试用例放进去吧

妥了

Copy link
Contributor

Choose a reason for hiding this comment

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

我看还没提交上来呀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我看还没提交上来呀

刚提交了

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2023

Codecov Report

Merging #5243 (35da27a) into develop (d19cfee) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop    #5243      +/-   ##
=============================================
- Coverage      48.48%   48.48%   -0.01%     
  Complexity      4139     4139              
=============================================
  Files            743      743              
  Lines          26520    26518       -2     
  Branches        3294     3294              
=============================================
- Hits           12857    12856       -1     
  Misses         12263    12263              
+ Partials        1400     1399       -1     
Impacted Files Coverage Δ
...seata/core/serializer/SerializerClassRegistry.java 0.00% <ø> (ø)
...o/seata/serializer/kryo/KryoSerializerFactory.java 90.90% <100.00%> (-0.40%) ⬇️
...n/src/main/java/io/seata/common/util/IdWorker.java 77.08% <0.00%> (-6.25%) ⬇️
...er/src/main/java/io/seata/server/ServerRunner.java 50.00% <0.00%> (ø)
...torage/file/store/FileTransactionStoreManager.java 56.27% <0.00%> (+0.64%) ⬆️
...ava/io/seata/server/metrics/MetricsSubscriber.java 15.65% <0.00%> (+0.86%) ⬆️

@funky-eyes
Copy link
Contributor

image

@funky-eyes funky-eyes changed the title optimize: 升级 kryo 5.4.0 优化了对jdk17的兼容性 optimize: kryo 5.4.0 optimize compatibility with jdk17 Jan 10, 2023
Copy link
Contributor

@funky-eyes funky-eyes left a comment

Choose a reason for hiding this comment

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

LGTM @jsbxyyx PTAL

@slievrly slievrly changed the title optimize: kryo 5.4.0 optimize compatibility with jdk17 optimize: kryo 5.4.0 compatibility with jdk17 Jan 29, 2023
Copy link
Member

@slievrly slievrly left a comment

Choose a reason for hiding this comment

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

LGTM

@slievrly slievrly merged commit 63f685e into apache:develop Jan 29, 2023
@wangliang181230 wangliang181230 added this to the 1.7.0 milestone Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants