-
Notifications
You must be signed in to change notification settings - Fork 81
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
Add support for external Avro Schema Registry #411
Conversation
@microsoft-github-policy-service agree |
@florianluediger - thanks for the contribution! How critical is this feature support? Is the support sufficient for C# alone? |
Hi @raorugan and thanks for looking into this PR! We need the support of a Schema Registry in our project so we don't need to specify our schemas manually and also we will be able to use objects with different schemas on the same topic. We would like to start using the extension in January. C# support would be sufficient for our project at the moment. |
Hi , Can you please add e2e tests with Confluent Kafka and Azure Event hubs along with this change ? |
Hi, I have added an e2e test that demonstrates the usage of the trigger and the binding. If you need any other cases covered, let me know. |
@raorugan did you have the time to look into the code again? |
@florianluediger, Thanks for the contribution. We are looking into it and will provide an update this week. |
@florianluediger Apologies for the delay in response - We had to arrange some internal discussions with wider teams to make sure the changes are fully compatible with the functions ecosystem. To summarise the discussion - Possible Solution - |
Thanks a lot for looking into my code! I will look into it again and change it accordingly. Until then, have a good time! |
…rs for trigger and output
Hi again @krishna-kariya @jainharsh98 I finally found the time to change my implementation. It now uses a parameter in the Kafka and KafkaTrigger Attributes to specify the URL of the schema registry. It would be great, if you could have another look even if it has been a long time since my last update on this PR. PS: Sorry for all the whitespace changes, let me know if I should revert those. I have kept them in here because they fix some formatting issues. |
@jainharsh98 @krishna-kariya You gave my comment a 👍. Does this mean that you are okay with my changes? Can you tell me about the next steps to get this PR merged? |
Hi @florianluediger, To use Confluent Schema registry, customers would need to provide the credentials for schema registry like API Key and Secret to the extension. Then only, the extension would be able to access the schema from the schema registry. Ref - here. Would you be willing to make additional changes to support schema registry related credentials? This would greatly improve the functionality of the feature. |
Hi @krishna-kariya, I don't have access to a Confluent Cloud account so I can't really test the authentication with API key and secret because my local testing environment does not support it. Also I am not sure how to implement this, because the class CachedSchemaRegistryClient that I am using does not seem to support authentication via API key and secret (unless I am misunderstanding something). I have added the option to provide basic authentication credentials so at least that is an option now. Would it be possible to integrate the current state of the feature into dev and move the authentication via key/secret to a future issue? This PR has been open for quite some time now (which is definitely my fault in many ways). |
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.
Overall, changes look good. One last comment - Can you resolve the attribute values before passing to serializer/deserializer?
src/Microsoft.Azure.WebJobs.Extensions.Kafka/Trigger/KafkaTriggerAttributeBindingProvider.cs
Outdated
Show resolved
Hide resolved
@krishna-kariya Sure, I have made the changes. |
/azp run |
No pipelines are associated with this pull request. |
No pipelines are associated with this pull request. |
Hi @florianluediger, Thanks for making these changes. I started the pipeline run for this PR and it is failing as Kafka cluster running for E2E tests doesn't have schema registry server. Can you add it to the docker compose here? |
/azp run |
No commit pushedDate could be found for PR 411 in repo Azure/azure-functions-kafka-extension |
@krishna-kariya I have added the Schema Registry to the Compose file. |
test/Microsoft.Azure.WebJobs.Extensions.Kafka.EndToEndTests/kafka-singlenode-compose.yaml
Show resolved
Hide resolved
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!
Closes #35
Hi there!
We need to be able to use a Confluent Schema Registry with this extension for one of our projects, so I added support for it in this PR. It would be great, if you could have a look at it and give me some feedback. I will try to respond as soon as I can if there are any changes that you want me to do.
Happy holidays!