-
Notifications
You must be signed in to change notification settings - Fork 901
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
JSON referenced schema support #1514
Conversation
ea82e94
to
f50d5f6
Compare
f50d5f6
to
e19e9d7
Compare
fd856a8
to
2f01818
Compare
2f01818
to
6f189ff
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.
Add changelog.
if named_schemas is None: | ||
named_schemas = {} | ||
for ref in schema.references: | ||
referenced_schema = schema_registry_client.get_version(ref.subject, ref.version) |
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 use a batch call for fetching the references?
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.
I checked on this, it doesn't looks like it's available directly through schema registry. https://docs.confluent.io/platform/current/schema-registry/develop/api.html#post--subjects-(string-%20subject)-versions
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.
In that case, can we parallelize the calls? Maybe in future.
if named_schemas is None: | ||
named_schemas = {} | ||
for ref in schema.references: | ||
referenced_schema = schema_registry_client.get_version(ref.subject, ref.version) |
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.
In that case, can we parallelize the calls? Maybe in future.
Fix build issues as well |
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.
Write some more tests. Unit test. Negative integration test.
Added a unit test. I believe the existing tests should cover the added code. Please let me know if you want me to add some more specific tests. |
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.
I would like a thorough test cases for this. Might be out of the scope of this PR but can we add that as well to the jira that we are creating to fix issues in this code?
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!
Just check the docs.
raise ValueError( | ||
"""schema_registry_client must be provided if "schema_str" is a Schema instance with references""") | ||
else: | ||
raise ValueError('You must pass either str or Schema') |
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.
ditto
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.
Thx, I've updated this.
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!
Remove this function and merge it.
@@ -43,6 +43,34 @@ def __exit__(self, *args): | |||
return False | |||
|
|||
|
|||
def _id_of(schema): |
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.
This function is not needed anymore!
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.
Missed removing it. Updated now.
* JSON referenced schema support * Changes * PR Feedback * PR Feedback * PR Feedback * PR Feedback * Fix wrong documentation * PR Feedback * Update unit tests * PR Feedback * Use ref.name as the id * Remove _id_of function
Changes to support referenced schemas in JSON schema.
Referenced schemas are always assumed to be registered in the SR prior.