Skip to content

Add support of references/dependencies for Protobuf schema type (DEPRECATED) #467

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

libretto
Copy link
Contributor

@libretto libretto commented Oct 5, 2022

What it does

These changes add basic support of references/dependencies for protobuf schema type.
This code contains much common code which allows adding support of references/dependencies to other types.
Limitations:
The compatibility check does not support references/dependencies.
Serialization/Deserialization of protobuf does not support references/dependencies.

References: #195

UPDATE: This PR already supports compatibility check (schema compare).

@libretto libretto requested review from a team as code owners October 5, 2022 20:43
Copy link
Contributor

@jjaakola-aiven jjaakola-aiven left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am getting 500 error with following request flow:

#!/bin/bash

NO_REF='{"schemaType":"PROTOBUF","schema":"syntax = \"proto3\";\n\nmessage NoReference {\n    string name = 1;\n}\n"}'
curl -H "Content-Type: application/json" localhost:8081/subjects/sub1/versions -d "$NO_REF"

WITH_FIRST_REF='{"schemaType":"PROTOBUF","schema":"syntax = \"proto3\";\n\nimport \"NoReference.proto\";\n\nmessage WithReference {\n    string name = 1;\n    NoReference ref = 2;\n}","references":[{"name":"NoReference.proto","subject":"sub1","version":1}]}'
curl -H "Content-Type: application/json" localhost:8081/subjects/sub2/versions -d "$WITH_FIRST_REF"

NO_REF_SECOND='{"schemaType":"PROTOBUF","schema":"syntax = \"proto3\";\n\nmessage NoReferenceTwo {\n    string name = 1;\n}\n"}'
curl -H "Content-Type: application/json" localhost:8081/subjects/sub3/versions -d "$NO_REF_SECOND"

ADD_NEW_REF_IN_SUB2='{"schemaType":"PROTOBUF","schema":"syntax = \"proto3\";\n\nimport \"NoReference.proto\";\nimport \"NoReferenceTwo.proto\";\n\nmessage WithReference {\n    string name = 1;\n    NoReference ref = 2;\n    NoReferenceTwo refTwo = 3;\n}\n","references":[{"name":"NoReference.proto","subject":"sub1","version":1},{"name":"NoReferenceTwo.proto","subject":"sub3","version":1}]}'
curl -H "Content-Type: application/json" localhost:8081/subjects/sub2/versions -d "$ADD_NEW_REF_IN_SUB2"

The usage of well known types should be demonstrated in tests and with the REST proxy.

@jjaakola-aiven jjaakola-aiven changed the base branch from main to protobuf-schema-references-support October 20, 2022 12:18
@jjaakola-aiven
Copy link
Contributor

jjaakola-aiven commented Oct 20, 2022

@libretto I will merge this to feature branch in Karapace and I will add some tests and changes on top of this. Please comment on the open questions, especially the need of .proto files.

Following work here: https://github.com/aiven/karapace/commits/protobuf-schema-references-support
Pull request #473 for visibility and commenting.

@libretto
Copy link
Contributor Author

libretto commented Oct 20, 2022

I just working on fixing issues. I just debugging them. Also, I will add a compatibility check to dependencies support as soon as one of fixes requires it.

@jjaakola-aiven
Copy link
Contributor

@libretto some of the issues are resolved in PR #473. Please do not update to this PR.

@libretto
Copy link
Contributor Author

@libretto some of the issues are resolved in PR #473. Please do not update to this PR.

@jjaakola-aiven all things which You resolved was resolved in my code too but i do not push it to GitHub while tests are not successful.
Now all tests are successful and I push it there. The next step is testing and polishing it.

@libretto
Copy link
Contributor Author

@jjaakola-aiven latest code from https://github.com/instaclustr/karapace/tree/pr2_clear works ok. So we could add Your tests there to see if it works. If something does not work let me fix the bugs.

@libretto
Copy link
Contributor Author

libretto commented Oct 26, 2022

@jjaakola-aiven When I open this PR I notice in the description its limitations and You know that I still working on it. I do not understand why You go to refactor and change code that is under my development without any discussion or requests for changes. I see You change the target branch for this PR to the draft branch and PR it to Main... But Your draft branch does not contain my latest code which includes fixes and recursive compatibility check which is already released in my code. The question is what I must do? Will You add this functionality by yourself so my branch and this PR are not required anymore so I may close my PR and will continue my next task on top of Your code? Or You will get my latest code and add Your changes to it so You will have all the functionality and fixes which I already implemented with Your changes?

@libretto
Copy link
Contributor Author

libretto commented Nov 1, 2022

@jjaakola-aiven i added there more tests from my branch which show bugs in dependencies compatibility algorithm which You implemented by self. And now I try debugging code which You offer there.

@jjaakola-aiven
Copy link
Contributor

@libretto Pick the #473 as a base, add missing features, tests, corrections on top of it. That will serve as the feature branch to land the profobuf reference support to Karapace.

@libretto
Copy link
Contributor Author

libretto commented Nov 9, 2022

@jjaakola-aiven please check and merge with Your branch

@sujayinstaclustr
Copy link

@jjaakola-aiven can you confirm if there is anything that needs to be done, we are waiting for you to approve this PR for merge.

@libretto
Copy link
Contributor Author

libretto commented Dec 19, 2022

@jjaakola-aiven I see red text with 1 change request but all change requests were resolved weeks ago. I fixed all bugs which we talk about there. But this PR was not accepted. Why?

@RyanSkraba RyanSkraba self-requested a review December 19, 2022 17:01
@RyanSkraba
Copy link
Contributor

Hello @libretto, I just wanted to keep you in the loop -- I'll be taking a closer look at this PR and #504 in order to get this moving for the next release of Karapace. Thanks for all the work so far!

I'm catching up a bit on this and the the work being done in #195

@libretto libretto closed this Mar 14, 2023
@libretto libretto changed the title Add support of references/dependencies for Protobuf schema type Add support of references/dependencies for Protobuf schema type (DEPRECATED) Mar 14, 2023
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.

4 participants