-
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
bugfix: kryo support circular reference #4978
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #4978 +/- ##
=============================================
+ Coverage 48.77% 48.79% +0.02%
- Complexity 4078 4081 +3
=============================================
Files 734 734
Lines 25853 25854 +1
Branches 3190 3190
=============================================
+ Hits 12609 12616 +7
+ Misses 11901 11898 -3
+ Partials 1343 1340 -3
|
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.
LGTM 标题改为英文标题 bugfix: xxxx,尽量简洁,并且再进行pr信息登记
complete |
你看下我这样改的标题是否正确,如果可以就把登记的pr信息重新改一下,言简意赅 |
已更改 |
@JavaLionLi rm-datasource KryoSerializerFactory fix too. |
@@ -48,6 +48,7 @@ public class KryoSerializerFactory { | |||
@Override | |||
protected Kryo create() { | |||
Kryo kryo = new Kryo(); | |||
kryo.setReferences(true); |
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 m wondering that why kryo 4 the property of references is true, and kryo 5 is false. What is the reason for example like lower performance.
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 you present a small test report?
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 you present a small test report?
This is not very clear
The kryo community didn't explain why
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 you present a small test report?
If not set, the stack will overflow
The scheme given by the community is set to true
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.
If not set, the stack will overflow The scheme given by the community is set to true
yes, the stack will overflow. But this problem look like never happen in seata, because the object from database it can not with cycle, so why I m wonder the performance if set it true.
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.
If not set, the stack will overflow The scheme given by the community is set to true
yes, the stack will overflow. But this problem look like never happen in seata, because the object from database it can not with cycle, so why I m wonder the performance if set it true.
EsotericSoftware/kryo#617
If complex objects are not involved, it is best not to enable them
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.
If not set, the stack will overflow The scheme given by the community is set to true
yes, the stack will overflow. But this problem look like never happen in seata, because the object from database it can not with cycle, so why I m wonder the performance if set it true.
EsotericSoftware/kryo#617 If complex objects are not involved, it is best not to enable them
- X is enabled by default, and it needs to be tested whether it will have an impact after being closed
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.
- X is enabled by default, and it needs to be tested whether it will have an impact after being closed
ok
Fixed |
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.
LGTM
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.
LGTM
麻烦请签署Seata社区的cla协议感谢配合 https://cla-assistant.io/seata/seata?pullRequest=5164 |
这是给dubbo提交pr的过程中遇到的问题 如下是kryo的官方回复
EsotericSoftware/kryo#919
Ⅰ. 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