Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

dmvk commented on PR #24586:
URL: https://github.com/apache/flink/pull/24586#issuecomment-2122091374

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

davidradl commented on code in PR #24586:
URL: https://github.com/apache/flink/pull/24586#discussion_r1582776111


##
flink-core/src/main/java/org/apache/flink/api/java/typeutils/runtime/NoFetchingInput.java:
##
@@ -73,17 +73,14 @@ protected int require(int required) throws KryoException {
 position = 0;
 int bytesRead = 0;
 int count;
-while (true) {
+while (bytesRead < required) {
 count = fill(buffer, bytesRead, required - bytesRead);

Review Comment:
   @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? 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

dannycranmer merged PR #24586:
URL: https://github.com/apache/flink/pull/24586


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

qinghui-xu commented on PR #24586:
URL: https://github.com/apache/flink/pull/24586#issuecomment-2039567737

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


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

flinkbot commented on PR #24586:
URL: https://github.com/apache/flink/pull/24586#issuecomment-2024784567

   
   ## CI report:
   
   * a37e562b54ed9ad4b9290f2f999542ea9104c65f UNKNOWN
   
   
   Bot commands
 The @flinkbot bot supports the following commands:
   
- `@flinkbot run azure` re-run the last Azure build
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[PR] [FLINK-34954][core] Kryo Input bug fix [flink]

[via GitHub]

qinghui-xu opened a new pull request, #24586:
URL: https://github.com/apache/flink/pull/24586

   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
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: issues-unsubscr...@flink.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org