feat: updated kafka topic configuration object #238
Conversation
There was a problem hiding this comment.
I would rather not bring vendor-specific extensions into the bindings like this.
Confluent are pretty good at keeping their platform-specific extensions, that aren't part of Kafka, separate - by prefixing the option names with confluent.
For example, they have confluent.key.schema.validation that you have described here as schema.key.validation.enabled.
I think that is a good pattern to adopt - while I absolutely see value in us including widely used vendor extensions in the bindings, I think they should be clearly identified as such. Not only that, if we match the names to what they are to use, we make the life of developers easier as they don't need to translate between naming formats.
I recommend copying the property names as-is. If you think the renaming is helpful (e.g. adding "enabled" to make it clearer it is a boolean) then I can live with that but I'd still prefer that we keep the "confluent." prefix.
|
Hi @dalelane, I've updated the property names as you suggested. It makes sense to keep that widely used vendor-specific config name as it is since it'll help developers when it comes to mapping those values. In general, I'm in favour of not leaking vendor-specific properties into the asyncapi bindings but I think it's acceptable to add some of the widely used ones and update additionalProperties as true for the less popular configs so toolings can consider parsing additional properties for the topicConfiguration object. |
|
Thanks for approving the PR. Once this one is merged I'll open the json schema PR (asyncapi/spec-json-schemas#481) for review. @dalelane |
|
Hi @derberg, I think this PR is ready for merge. Do you want to merge it or wait for more reviewers? |
|
Hey @lbroudoux, would you like to review/merge these changes? |
|
/rtm |
Description
Related issue(s)
#231