-
Notifications
You must be signed in to change notification settings - Fork 2.5k
[HUDI-837]: implemented custom deserializer for AvroKafkaSource #1562
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
Conversation
|
Thinking of writing test cases for this, but unable to simulate because AbstractKafkaAvroDeserializer expects a working schema-registry url. Not sure of how to mock the same here since it is library class. |
|
@afilipchik @umehrot2 help review this? :) |
|
@afilipchik interested in reviewing this? |
...ities/src/main/java/org/apache/hudi/utilities/sources/serde/HoodieAvroKafkaDeserializer.java
Show resolved
Hide resolved
...ities/src/main/java/org/apache/hudi/utilities/sources/serde/HoodieAvroKafkaDeserializer.java
Outdated
Show resolved
Hide resolved
...ities/src/main/java/org/apache/hudi/utilities/sources/serde/HoodieAvroKafkaDeserializer.java
Show resolved
Hide resolved
...ities/src/main/java/org/apache/hudi/utilities/sources/serde/HoodieAvroKafkaDeserializer.java
Outdated
Show resolved
Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/AvroKafkaSource.java
Outdated
Show resolved
Hide resolved
|
@vinothchandar @afilipchik Let us close this? :) |
vinothchandar
left a comment
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.
Can we add a test around this ? I was kind of surprised that the reading of the avro records based on latest schema is not happening using the existing deserializer..
Does everyone out there write this code themselveS? or is there a deserializer that we can use alreayd?
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/AvroKafkaSource.java
Show resolved
Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/sources/AvroKafkaSource.java
Outdated
Show resolved
Hide resolved
hudi-utilities/src/main/java/org/apache/hudi/utilities/UtilHelpers.java
Outdated
Show resolved
Hide resolved
We can just mock the response it will send into a test SchemaProvider.. We need not mock SchemaRegistry itself |
|
@n3nash can you review this and take it home? |
|
@n3nash please take a look. It is good to merge now. All the comments are addressed. |
|
@n3nash got a chance to look at this? :) |
|
@n3nash @vinothchandar I guess we can merge this? :) |
...ities/src/main/java/org/apache/hudi/utilities/sources/serde/HoodieAvroKafkaDeserializer.java
Outdated
Show resolved
Hide resolved
|
@n3nash Please take a pass. |
|
Closing this in favor of #2619 |
…e#1562) Co-authored-by: Tien <[email protected]>
Tips
What is the purpose of the pull request
Brief change log
Verify this pull request
(Please pick either of the following options)
This pull request is a trivial rework / code cleanup without any test coverage.
(or)
This pull request is already covered by existing tests, such as (please describe tests).
(or)
This change added tests and can be verified as follows:
(example:)
Committer checklist
Has a corresponding JIRA in PR title & commit
Commit message is descriptive of the change
CI is green
Necessary doc changes done or have another open PR
For large changes, please consider breaking it into sub-tasks under an umbrella JIRA.