-
Notifications
You must be signed in to change notification settings - Fork 22
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
CNDB-11118 return null serializer if response verb is null #1328
base: main
Are you sure you want to change the base?
Conversation
6420124
to
0bb6918
Compare
@Test | ||
public void testInternalResponseNullSerializer() throws Exception | ||
{ | ||
depractedResponsesShouldReturnNullSerializer(Verb.INTERNAL_RSP); |
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.
typo: deprecated (few places
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.
Looks good in general. I wonder if it is possible to add a unit test that exercise full encode/decode to verify the consequences of returning a null serializer().
<In,Out> IVersionedAsymmetricSerializer<In, Out> responseSerializer(long id, InetAddressAndPort peer) | ||
{ | ||
CallbackInfo info = get(id, peer); | ||
return info == null ? null : info.responseVerb.serializer(); | ||
return info == null || info.responseVerb == null ? null : info.responseVerb.serializer(); |
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 it would be good to have a comment here to explain why this is being done.
5c0c610
to
5226175
Compare
Thank you, I applied the review remarks.
This would mean opening access to a bunch of methods. I decided against it with this implementation. |
5226175
to
0df5f8a
Compare
response verb is null for deprecated REQUEST_RSP and INTERNAL_RSP. info.responseVerb.serializer() caused NullPointerExceptions. The null serializer is later gracefully handled in deserializePayloadPre40.
0df5f8a
to
35eb670
Compare
Quality Gate passedIssues Measures |
❌ Build ds-cassandra-pr-gate/PR-1328 rejected by Butler1 new test failure(s) in 2 builds Found 1 new test failures
Found 11 known test failures |
response verb is null for deprecated REQUEST_RSP and INTERNAL_RSP. info.responseVerb.serializer() caused NullPointerExceptions.
The null serializer is later gracefully handled in deserializePayloadPre40.