search

obsidiandynamics / kafdrop

Kafka Web UI
Apache License 2.0
5.56k stars 841 forks source link

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

Open nicklester opened 1 year ago

nicklester commented 1 year ago

Added support to enable the AWS Glue Schema Registry

davideicardi commented 1 year ago

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 ?

nicklester commented 1 year ago

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 :)

Bert-R commented 1 year ago

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.