avi-sanwal commented on PR #28161: URL: https://github.com/apache/flink/pull/28161#issuecomment-5290848436
Thanks for taking a look and for spelling these out. I pushed a commit with the diagnostic and negative-test follow-ups. > 1. Per-record allocation in the hot path (performance) The extra schema-resolution work only runs when the GenericRecord schema differs from the serializer runtime schema; matching schemas continue through the existing path unchanged. I also deliberated caching the migration writer/reader, but caching the writer would not remove the field traversal or conversion work. I left that out to avoid adding retention and complexity to a migration-only path. > 2. No IOException wrapping / error message Added wrapping that preserves the original cause and includes both the writer and reader schemas in the error message. > 3. Objects.equals on Avro Schema I checked Avro Schema.equals(): it already performs an identity check before structural comparison, so an explicit recordSchema == runtimeSchema || ... check would be redundant. > 4. Test helper methods serialize() / deserialize() — already in the test class? Confirmed—the existing helpers exercise AvroSerializer.serialize() and deserialize(). The test follows the relevant migration sequence: deserialize with the previous serializer, then serialize with the current one. > 5. No coverage for incompatible schema (negative test) Added a negative test for an old record that is missing a required field in the reader schema. It verifies the contextual IOException and preserves the underlying AvroTypeException as the cause. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
