Basic support of references Karapace API calls. #21
Conversation
libretto
changed the title
|
@libretto I merged the latest changes from avien/karapace upstream in to instaclustr/karapace. It had merge conflict and they removed ujson dependency. Please update your branches accordingly if you have any. I have made the changes to the deps branches and PR is now updated with upstream. |
There was a problem hiding this comment.
Sorry for the delay on reviewing this.
I've done first pass and some comments. I have not looked the testing coverage yet.
Compatibility check API does not support references, /compatibility/subjects/<subject:path>/versions/<version:path>. I am not sure how CSR uses it, but worth checking.
|
|
||
|
|
||
| class References: | ||
| def __init__(self, schema_type: SchemaType, references: JsonData): |
There was a problem hiding this comment.
I would propose to type the references to specific type instead of JsonData.
There was a problem hiding this comment.
@jjaakola-aiven Which reason to add some more types there when the specific type is "References" and data for this type is stored in Kafka in JSON?
There was a problem hiding this comment.
Probably just an error from me on review, not sure after vacation. Possibly I've been thinking on modeling the data directly in the controller layer from JSON to objects.
| new_schema_references = None | ||
| references = body.get("references") | ||
| if references: | ||
| if schema_type != SchemaType.PROTOBUF: |
There was a problem hiding this comment.
This should be validated before calling write_new_schema_local.
There was a problem hiding this comment.
@jjaakola-aiven Functionality of references validation must be released in next PR. Now we add basic functionality only
fixed docstring
fiexed equation functions
change line feed
Co-authored-by: Jarkko Jaakola <[email protected]>
|
@jjaakola-aiven We addressed some of the feedback and waiting for to discuss few open comments (the ones showing pending) |
|
|
@libretto there are conflicts in the code pushed. Can you resolve the conflicts? |
|
@jjaakola-aiven If this PR is all good to merge and we can raise a PR to upstream |
| ref_str = reference_key(ref["subject"], ref["version"]) | ||
| referents = self.referenced_by.get(ref_str, None) | ||
| if referents: | ||
| LOG.info("Adding entry subject referenced_by : %r", ref_str) |
There was a problem hiding this comment.
Must we remove it?
There was a problem hiding this comment.
If deemed valuable for debugging no need to remove, but change it to LOG.debug.
| LOG.info("Adding entry subject referenced_by : %r", ref_str) | ||
| referents.append(schema_id) | ||
| else: | ||
| LOG.info("Adding entry subject referenced_by : %r", ref_str) |
|
|
||
| res = await registry_async_client.delete("subjects/customer/versions/1") | ||
| assert res.status_code == 404 | ||
| match_msg = "Subject 'customer' Version 1 was not deleted because it is referenced by schemas with ids:[2]" |
There was a problem hiding this comment.
The lifecycle of schema and references would be nice to test:
- create schema A
- create schema B referencing A
- try to delete A
- delete B
- delete A
Steps 1-3 are covered, steps 4 and 5 would be needed.
karapace/schema_registry_apis.py
Outdated
| @@ -55,6 +60,7 @@ class SchemaErrorMessages(Enum): | |||
| "forward, full, backward_transitive, forward_transitive, and " | |||
| "full_transitive" | |||
| ) | |||
| REFERENCES_SUPPORT_NOT_IMPLEMENTED = "Schema references are not supported for '{schema_type}' schema type yet" | |||
There was a problem hiding this comment.
The yet is not needed and the request could contain arbitrary string as schema type resulting Schema references are not supported for '<ARBITRARY>' schema type yet
There was a problem hiding this comment.
The request always contains JSON with the exact schema type definition from the set of AVRO, JSON, and PROTOBUF in other case we raise an error.
There was a problem hiding this comment.
I meant that yet points to future support. I would just state in the error that not supported without hinting on possible future support.
Co-authored-by: Jarkko Jaakola <[email protected]>
karapace/schema_registry_apis.py
Outdated
| if referenced_by and len(referenced_by) > 0: | ||
| self.r( | ||
| body={ | ||
| "error_code": SchemaErrorCodes.SCHEMAVERSION_HAS_REFERENCES.value, |
There was a problem hiding this comment.
I cross checked this error code and correct would be: https://github.com/confluentinc/schema-registry/blob/master/core/src/main/java/io/confluent/kafka/schemaregistry/rest/exceptions/Errors.java#L63
| @@ -67,6 +68,10 @@ class InvalidSchema(Exception): | |||
| pass | |||
|
|
|||
|
|
|||
| class InvalidReferences(Exception): | |||
We add basic support of references to following API calls:
POST /subjects/(string: subject)/versions
POST /subjects/(string: subject)
GET /subjects/(string: subject)/versions/{versionId: version}/referencedby
DELETE /subjects/subject:path/versions/version:path
DELETE /subjects/subject:path
Support of references is limited to PROTOBUF schema type, but this code will be reused with minimal changes in the future implementation of references to other schemas.