-
Notifications
You must be signed in to change notification settings - Fork 917
Mirror subject.name.strategy from Java client #401
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
It looks like @delphyne hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
[clabot:check] |
@confluentinc It looks like @delphyne just signed our Contributor License Agreement. 👍 Always at your service, clabot |
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.
Thanks for this @delphyne!
I have a few requests around the implementation but overall your approach looks good.
@rnpridgeon are there any further changes you'd like to see on this PR? |
Would be wonderful if this can be merged by someone with write access. @rnpridgeon |
Provides a means to configure a subject name strategy for both key and value when using the schema registry. This brings the python client in line with the java client, and enables the use of multiple schemas on a single topic.
* Remove plugin registration and lookup * Remove interface in favor of callback
bac9f62
to
81f3fa7
Compare
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.
Let's get rid of the changes to setup.py, move the _serializer setter below the super class initialization and add some integration tests to examples/integration_test.py.
Other than that LGTM
* Add integration tests * Fix potential issue using RecordNameStrategy with primitive or null schemas
* Move serializer setter below superclass ctor call
@delphyne is this PR still alive? |
I believe I have addressed all feedback from @rnpridgeon, but he has not commented since July 14. I am uncertain if he or @edenhill intend to accept this contribution or not. |
@rnpridgeon merge conflicts have been resolved. The travis build says it failed, but it's actually passing... there was a step that failed because of a sha-hash mismatch with an apt-get installed package, but when i reran it passed fine. However, it did not update the status on the PR when it succeeded. |
Confluent Platform needs to allow for data with different schemas to be present in a single topic - people are moving past the simple data integration and buffering use cases of Kafka, and this feature enables an important design pattern (that we're evangelizing) that will become increasingly requested. Allowing people to do this is table-stakes for a streaming platform IMO. What we probably want to do instead is get associated changes made to connect. I haven't thought about the specifics in any depth at all - it's possible the @ept's changes are not the optimal way to go about this when viewed from a CP wide context, but would guess they probably are, and if not, well python is in the same boat as java ... |
Quick clarification: You can have multiple schemas in a single topic they just need to be compatible with one another. This is an important distinction as we talk about the scope of schema lineage enforcement. I see your point though @mhowlett and given that this will likely be coming to .NET in the near future as well I will reevaluate my decision. I think there are are a few supporting features that may need to come into the picture as well in order to do this truly effective. I need to wrap up 1.0 and a few other things but after that I will put some careful thought into how to make this work given the state of the Avro Python implementation. From there we can reevaluate the existing PRs and make a plan on how to proceed. @delphyne sorry for falling behind on this, I will circle back with you after the release to see what we can do to move this forward. |
FWIW, I'd also like to chime in that this feature request is not only about "multiple schemas in same topic". I think there are also other use cases where one wants to go beyond the 1-on-1 coupling of schema and topic:
Now that we switched our app to the "record name strategy" things seem to make more sense and the "topic name strategy" feels like a weird default actually. |
This will be covered with generic serde support, #502 |
Provides a means to configure a subject name strategy for both key and value when using
the schema registry. This brings the python client in line with the java client, and
enables the use of multiple schemas on a single topic.