kafdrop icon indicating copy to clipboard operation
kafdrop copied to clipboard

WIP - Do not Merge: Added support for AWS Glue Schema registry

Open nicklester opened this issue 2 years ago • 3 comments

Added support to enable the AWS Glue Schema Registry

nicklester avatar Mar 21 '23 11:03 nicklester

Thank you @nicklester ! It LGTM, maybe I would only prefer to have the AWS Glue implementation a little more "isolated" ... What do you think @Bert-R ?

davideicardi avatar Mar 24 '23 08:03 davideicardi

I've added a few small comments. I agree with @davideicardi that a bit more isolation and genericity would be helpful.

What about this:

  • Add a property schemaregistry.type, with possible values generic and aws-glue. Default is generic.
  • Extend SchemaRegistryProperties with a check: if type != 'generic', then the properties connect and auth should not be set.
  • Enhance GlueSchemaRegistryProperties in a similar way and remove isConfigured. Als add a validation that the mandatory properties are set if this registry type is configured.
  • Extend AvroMessageDeserializer with a map of deserializer factories, keyed by the registry type.

With that, we're prepared for a future PR that adds support for Microsoft's SchemaRegistryApacheAvroSerializer

Was just a quick question - (as Java is not my 1st or even 2nd language!). Isn't using a factory going to mean we need to pass the same types as arguments, but the generic and glue configs are different, and in different hierarchies). Apologies if I've missed something obvious :)

nicklester avatar Mar 29 '23 09:03 nicklester

No problem! As always, there are multiple ways to approach it. I'd do a slightly bigger refactor as you find in the attached patch. Note that I have not done any testing. The constants for the schema registry type would need to be moved from MessageDeserializerFactory to a SchemaRegistryConfiguration, to use them to test the possible values for type.

Bert-R avatar Mar 29 '23 19:03 Bert-R