[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user cricket007 commented on the issue: https://github.com/apache/flink/pull/5995 What about implementing a `KeyedDeserializationSchema` for Avro? ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5995 Looks good, thanks! +1 to merge this ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5995 I've addressed your comments @StephanEwen . If you don't have any more, I will merge it today. ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5995 Added a few more comment, most importantly around exception wrapping. Otherwise, looking good... ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5995 @StephanEwen could you have another look? ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5995 I would actually keep the package name for now. It makes sense, because the connection to the registry is avro-specific at the moment... ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5995 Also as for the package name or place where to put it, I don't feel competent to suggest a place, therefore will be happy to apply your suggestion. ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5995 As for the snapshot binary data, I do understand that it should be created with appropriate flink version (in this case in theory with flink 1.3) and I've tried really hard to do so until I found out that this test is incompatible with 1.3 and the data could not be generated with flink 1.3 Later found out the comment to the test class that also states so: > Important: Since Avro itself broke class compatibility between 1.7.7 (used in Flink 1.3) > * and 1.8.2 (used in Flink 1.4), the Avro by Pojo compatibility is broken through Avro already. > * This test only tests that the Avro serializer change (switching from Pojo to Avro for Avro types) > * works properly. Also the commented code does not compile with flink 1.3(but this is a minor thing) Data serialized with version of avro used in flink 1.3 (1.7.7) is not binary compatible with avro 1.8.2 (in flink 1.4+), due to changes how SpecificFixed is constructed. Therefore how I regenerated this snapshot data is that I run the commented code on current branch. That is why I also changed few descriptions to that test as it test compatibility of `PojoSerializer` with `AvroSerializer` rather than binary backwards compatibility. Nevertheless I am more than happy to hear any comments on that. ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user dawidwys commented on the issue: https://github.com/apache/flink/pull/5995 Right sorry for that. I've changed the data generator a bit, so it produced different results than before with the same seed. I've recreated the serialized data with updated `TestDataGenerator`. It took me a while though to figure out that it should be created with current code rather than 1.3 branch. Therefore I updated the comment accordingly. Also reworded a bit other names as the `BackwardsCompatibleAvroSerializerTest` does not test compatibility with 1.3, but only with `PojoSerializer`. ---
[GitHub] flink issue #5995: [FLINK-9337] Implemented AvroDeserializationSchema
Github user StephanEwen commented on the issue: https://github.com/apache/flink/pull/5995 Thanks, the main code looks good! Unfortunately, this seems to wither break the compatibility with prior savepoints (when Avro types were implicitly handled through Kryo, now bridged through the `BackwarsCompatibleAvroSerializer`) or needs to adjust that test. There are also some license header issues, causing the build to fail. ---
