Skip to content

Conversation

qinghui-xu
Copy link
Contributor

Handle edge case of zero length serialized bytes correctly.

What is the purpose of the change

Bug fix in kryo (NoFetching)Input implementation to handle properly zero length serialized bytes, eg the serialization of a protobuf message with default values.

Brief change log

  • Fix while loop for NoFetchingInput#read(byte[], int, int) and NoFetchingInput#require(int)

Verifying this change

This change is a trivial rework / code cleanup without any test coverage.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): no
  • The public API, i.e., is any changed class annotated with @Public(Evolving): no
  • The serializers: yes
  • The runtime per-record code paths (performance sensitive): yes
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no

Handle edge case of zero length serialized bytes correctly.
@flinkbot
Copy link
Collaborator

flinkbot commented Mar 28, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

@qinghui-xu
Copy link
Contributor Author

Hello team,
Could I get a review for this, please?

Copy link
Contributor

@dannycranmer dannycranmer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @qinghui-xu

@dannycranmer dannycranmer merged commit 3977982 into apache:master Apr 19, 2024
int count;
while (true) {
while (bytesRead < required) {
count = fill(buffer, bytesRead, required - bytesRead);
Copy link
Contributor

Choose a reason for hiding this comment

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

@dannycranmer I saw this linked to in slack - I wondered whether we could have junits for the required case and higher than required? Maybe as a follow on issue / pr ? I could code if you were willing to merge for me?

@dmvk
Copy link
Member

dmvk commented May 21, 2024

Why is this not covered with a regression test? That feels rather scary in such a critical part of the stack.

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