-
Notifications
You must be signed in to change notification settings - Fork 26.4k
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
Fix ContentTypeId conflict #3967
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3967 +/- ##
============================================
+ Coverage 63.58% 63.58% +<.01%
Complexity 100 100
============================================
Files 720 720
Lines 31838 31839 +1
Branches 5113 5114 +1
============================================
+ Hits 20245 20246 +1
Misses 9273 9273
Partials 2320 2320
Continue to review full report at Codecov.
|
/** | ||
* Serizlization ContentTypeId | ||
*/ | ||
public static final byte AVRO_SERIALIZATION_ID = 1; |
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.
this is wrong, avro is 10...
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 final byte AVRO_SERIALIZATION_ID = 11 should be better
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 final byte AVRO_SERIALIZATION_ID = 11 should be better
What are the uses of 1 and 5 here?
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.
let's simply leave it as is. I believe the original intention is: 1, 2, 4, 8, 16...
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 modified it.
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.
pls. check my comments.
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.
public static final byte PROTOSTUFF_SERIALIZATION_ID = 10; | ||
public static final byte AVRO_SERIALIZATION_ID = 11; | ||
public static final byte GSON_SERIALIZATION_ID = 16; | ||
|
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.
Maybe you can define a sub-constant class. What do you think?
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.
+1 for a specific constants class
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 think this suggestion is good, I will modify it later.
What is the purpose of the change
Fix Serialization ContentTypeId conflict between avro protocol and protocoluff protocol #3926
Brief changelog
XXXXX
Verifying this change
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
[Dubbo-XXX] Fix UnknownException when host config not exist #XXX
. Each commit in the pull request should have a meaningful subject line and body.mvn clean install -DskipTests=false
&mvn clean test-compile failsafe:integration-test
to make sure unit-test and integration-test pass.