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

CNDB-11118 return null serializer if response verb is null #1328

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jtgrabowski
Copy link

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.

@jtgrabowski jtgrabowski changed the base branch from august-release-hotfix-cndb-10596 to main October 17, 2024 12:09
@jtgrabowski jtgrabowski requested a review from a team October 17, 2024 12:13
@Test
public void testInternalResponseNullSerializer() throws Exception
{
depractedResponsesShouldReturnNullSerializer(Verb.INTERNAL_RSP);

Choose a reason for hiding this comment

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

typo: deprecated (few places

Copy link

@aymkhalil aymkhalil left a 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();
Copy link

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.

@jtgrabowski
Copy link
Author

Thank you, I applied the review remarks.

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().

This would mean opening access to a bunch of methods. I decided against it with this implementation.

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.
Copy link

sonarcloud bot commented Nov 4, 2024

@cassci-bot
Copy link

❌ Build ds-cassandra-pr-gate/PR-1328 rejected by Butler


1 new test failure(s) in 2 builds
See build details here


Found 1 new test failures

Test Explanation Branch history Upstream history
...ToolEnableDisableBinaryTest.testMaybeChangeDocs regression 🔴🔵 🔵🔵🔵🔵🔵🔵🔵

Found 11 known test failures

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants