Skip to content

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

Closed
wants to merge 6 commits into from

Conversation

delphyne
Copy link

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.

@ghost
Copy link

ghost commented Jun 14, 2018

It looks like @delphyne hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@delphyne
Copy link
Author

[clabot:check]

@ghost
Copy link

ghost commented Jun 14, 2018

@confluentinc It looks like @delphyne just signed our Contributor License Agreement. 👍

Always at your service,

clabot

Copy link
Contributor

@rnpridgeon rnpridgeon 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 @delphyne!

I have a few requests around the implementation but overall your approach looks good.

@rnpridgeon rnpridgeon requested a review from edenhill June 15, 2018 17:36
@delphyne
Copy link
Author

@rnpridgeon are there any further changes you'd like to see on this PR?

@nao921
Copy link

nao921 commented Jul 13, 2018

Would be wonderful if this can be merged by someone with write access. @rnpridgeon

delphyne added 2 commits July 13, 2018 07:45
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
@delphyne delphyne force-pushed the subject-name-strategy branch from bac9f62 to 81f3fa7 Compare July 13, 2018 12:45
Copy link
Contributor

@rnpridgeon rnpridgeon left a 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

delphyne added 2 commits July 17, 2018 13:33
* Add integration tests
* Fix potential issue using RecordNameStrategy with primitive or null schemas
* Move serializer setter below superclass ctor call
@soxofaan
Copy link
Contributor

soxofaan commented Dec 7, 2018

@delphyne is this PR still alive?
Thanks for your work, we could really use this feature.

@delphyne
Copy link
Author

delphyne commented Dec 7, 2018

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.

@delphyne
Copy link
Author

delphyne commented Dec 7, 2018

@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.

@mhowlett
Copy link
Contributor

mhowlett commented Dec 7, 2018

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 ...

@rnpridgeon
Copy link
Contributor

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.

@soxofaan
Copy link
Contributor

soxofaan commented Dec 9, 2018

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:

  • same schema on multiple topics
  • bake schema (id) at compile time in your app and determine topic at run time

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.

@rnpridgeon
Copy link
Contributor

This will be covered with generic serde support, #502

@rnpridgeon rnpridgeon closed this Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants