Skip to content

Conversation

@congbobo184
Copy link
Contributor

Motivation

Consumer can getValue return null
fix the #4803

Verifying this change

Add the tests for it

Does this pull request potentially affect one of the following parts:
If yes was chosen, please highlight the changes

Dependencies (does it add or upgrade a dependency): (no)
The public API: (no)
The schema: (yes)
The default values of configurations: (no)
The wire protocol: (no)
The rest endpoints: (no)
The admin cli options: (no)
Anything that affects deployment: (no)

Documentation

Does this pull request introduce a new feature? (yes / no)
If yes, how is the feature documented? (not applicable / docs / JavaDocs / not documented)
If a feature is not applicable for documentation, explain why?
If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@congbobo184
Copy link
Contributor Author

run Integration Tests

@congbobo184
Copy link
Contributor Author

run java8 tests

@congbobo184
Copy link
Contributor Author

run Integration Tests

@congbobo184
Copy link
Contributor Author

run java8 tests

}
} else {
return schema.decode(getData());
if (getData().length == 0) {
Copy link
Member

Choose a reason for hiding this comment

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

@congbobo184

I think the fix here is a bit problematic. because an application can produce a zero-length bytes array. Can we find a way to differentiate a zero-length bytes payload from a null payload?

Copy link
Member

Choose a reason for hiding this comment

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

we should use metadata to store the null payload.

@sijie sijie added this to the 2.5.0 milestone Aug 5, 2019
@sijie
Copy link
Member

sijie commented Nov 25, 2019

@congbobo184 are you still working on this change?

@congbobo184
Copy link
Contributor Author

I am not working on it.It can be moved to the next version.

@codelipenghui
Copy link
Contributor

Close this PR since #6379 has fixed the problem.

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.

5 participants