Re: [PR] [FLINK-34954][core] Kryo Input bug fix [flink]
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]
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]
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]
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]
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]
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