Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Avro Schema registry kafka pubsub implementation #3292

Merged
merged 6 commits into from Jan 10, 2024
Merged

Avro Schema registry kafka pubsub implementation #3292

merged 6 commits into from Jan 10, 2024

Conversation

passuied
Copy link
Contributor

@passuied passuied commented Dec 30, 2023 •
edited

Description

Add Avro Schema Registry to Kafka PubSub.

  • Leverages srclient and go-avro under the hood
  • Please note that for the serialization/deserialization to fully compatible, dates/datetimes in the JSON data need to be set as their respective Unix int and long representations, instead of the more standard Iso8601 one. This is a limitation of the goavro library.

Issue reference

#3144

Checklist

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation - Created issue

@passuied passuied requested review from a team as code owners December 30, 2023 00:53
@passuied passuied mentioned this pull request Dec 30, 2023
Closed
@passuied passuied force-pushed the feature/kafka-pubsub-schema-registry branch 2 times, most recently from ac09299 to 7b19c5b Compare January 1, 2024 18:21
@passuied passuied changed the title Avro Schema registry consumer implementation Avro Schema registry kafka pubsub implementation Jan 1, 2024
@passuied passuied force-pushed the feature/kafka-pubsub-schema-registry branch 7 times, most recently from 03e500d to 45d86a7 Compare January 4, 2024 17:36
@yaron2
Copy link
Member

yaron2 commented Jan 4, 2024

Great contribution, I really like how this is done and wonder if this approach can be used in the future to enable a generic schema validation feature for all Dapr pub/subs, implemented in the Dapr runtime itself.

For this PR: It'd be best to add a certification test for this functionality (or add this to an existing test).

@passuied
Copy link
Contributor Author

passuied commented Jan 4, 2024 •
edited

For this PR: It'd be best to add a certification test for this functionality (or add this to an existing test).

I see the existing test for kafka. Should be able to add to it easily. This is essentially how I'm already testing it locally anyway...

@ItalyPaleAle
Copy link
Contributor

The certification tests for Kafka PubSub are here: https://github.com/dapr/components-contrib/tree/main/tests/certification/pubsub/kafka

We don't have good docs (we should....), but for certifications test you just "cd" into the test's folder, and then run go test -v -tags cerrttests

@passuied passuied force-pushed the feature/kafka-pubsub-schema-registry branch 8 times, most recently from cf962f2 to c350f0e Compare January 6, 2024 17:50
@passuied
Avro Schema registry implementation
8ce3477 
- consumer
- producer
- includes configurable caching
- Overrides srclient.codec to use newer standard json codec since this is the format used by dapr.
- unit tests

Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
@passuied
Added avro test to pubsub/kafka test certification
42e166d 
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
@passuied passuied force-pushed the feature/kafka-pubsub-schema-registry branch from c350f0e to 42e166d Compare January 8, 2024 19:47
@yaron2
Copy link
Member

yaron2 commented Jan 9, 2024

@passuied please resolve the conflict

@passuied
Merge branch 'main' into feature/kafka-pubsub-schema-registry
4e5596f 
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
@passuied
formatting
e39b6c7 
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
@passuied passuied force-pushed the feature/kafka-pubsub-schema-registry branch from dde6ec9 to e39b6c7 Compare January 9, 2024 17:56
ItalyPaleAle
ItalyPaleAle requested changes Jan 9, 2024
View reviewed changes
Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR :) left some comments

pubsub/kafka/metadata.yaml Outdated Show resolved Hide resolved
bindings/kafka/metadata.yaml Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/kafka.go Outdated Show resolved Hide resolved
common/component/kafka/metadata.go Outdated Show resolved Hide resolved
@passuied
Addressed PR feedback
5a28441 
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
@ItalyPaleAle
Copy link
Contributor

LGTM! @yaron2 good to merge if you are good too

@yaron2 yaron2 added this pull request to the merge queue Jan 10, 2024
Merged via the queue into dapr:main with commit 419f03f Jan 10, 2024
88 checks passed
@yaron2
Copy link
Member

yaron2 commented Jan 10, 2024

Thanks for this @passuied

@passuied
Copy link
Contributor Author

Thanks for this @passuied

Thank you for a very smooth PR process ;) That was fun!

@passuied passuied deleted the feature/kafka-pubsub-schema-registry branch January 10, 2024 21:06
toneill818 pushed a commit to toneill818/components-contrib that referenced this pull request Jan 22, 2024
@passuied @ItalyPaleAle @toneill-newinnov
Avro Schema registry kafka pubsub implementation (dapr#3292)
58087d9 
Signed-off-by: Patrick Assuied <patrick.assuied@elationhealth.com>
Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com>
Signed-off-by: Thomas O'Neill <toneill@new-innov.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ItalyPaleAle ItalyPaleAle ItalyPaleAle approved these changes

@yaron2 yaron2 Awaiting requested review from yaron2

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@passuied @yaron2 @ItalyPaleAle