-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor deserialization to be safer #82
Conversation
@@ -104,6 +104,7 @@ object AvroUtils extends LazyLogging { | |||
headers | |||
) | |||
) | |||
} | |||
|
|||
implicit class RichGenericRecord(genericRecord: GenericRecord) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can replace the whole toSpecific method with this:
def toSpecific[A <: GenericRecord](instance: A): A =
SpecificData.get().deepCopy(instance.getSchema, genericRecord).asInstanceOf[A]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested it briefly and it seems to work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems to work meaning, the way it's working now. I do have an issue though with logical type conversions. Still trying to figure it out. It's not an issue with your changes. I think it's an avrohugger limitation and/or something else I'm missing. I'll explain in slack.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's brilliant! Good find.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this can fail, we should wrap it in a Try
, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
6c16b49
to
246638b
Compare
Several changes to deserialization to make it more robust
ConfluentAvroRegistryKafkaRecordDeserializationSchema
now respects thespecific.avro.reader
config. If you do not specify a setting, one will be selected based on whether the embedded avro record class is Generic or Specific. If you specifyspecific.avro.reader = false
and the embedded avro record class is Specific, the schema will deserialize as a Generic record, then attempt to convert to a Specific record instance.AvroUtils.toEmbeddedAvroInstance
) is now wrapped in aTry
, as it may fail.AvroUtils.toSpecific
utility method implementation with call toSpecificData.deepCopy()
.GlueAvroRegistryKafkaRecordDeserializationSchema
has a smarter key deserializer that will fallback to Confluent if you've configured a confluent fallback (so that string keys are properly processed).