Skip to content
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

CORE-6860 SR: Wire up support for ?verbose compatibility checks #22877

Merged
merged 8 commits into from
Aug 20, 2024

Conversation

pgellert
Copy link
Contributor

@pgellert pgellert commented Aug 14, 2024

This implements minimal support for the verbose flag on the compatibility check endpoints in schema registry. It includes rpk support. In redpanda, this introduces reporting more verbose information about the compatibility check performed. Currently only three new lines are reported when there is an incompatibility that show the version plus definition of the schema we are comparing against and the compatibility level being used. With this PR we don't yet support reporting the name of the specific compatibility check that was violated, but we implement much of the wiring to be able to do that in a follow-up PR.

This PR was cherry-picked and then adapted from @oleiman's earlier PR #18332.

Example rpk output on incompatibility:

% rpk registry schema check-compatibility sub1 --schema-version latest --schema /tmp/3.proto
Schema is not compatible.
{oldSchemaVersion: 3}
{oldSchema: 'syntax = "proto3";

message Simple {
  int32 id = 1;
  int32 new_id2 = 3;
}

'}
{compatibility: BACKWARD}

Fixes https://redpandadata.atlassian.net/browse/CORE-6860

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.2.x
  • v24.1.x
  • v23.3.x

Release Notes

Features

  • Schema Registry: added support for the "verbose" query parameter on the schema compatibility checker endpoint

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Copy link
Member

@oleiman oleiman left a comment

Choose a reason for hiding this comment

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

lgtm. leaving approval flag off b/c I wrote some of this code.

src/v/pandaproxy/schema_registry/types.h Show resolved Hide resolved
src/v/pandaproxy/schema_registry/compatibility.h Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/avro.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/compatibility.h Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/compatibility.h Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/compatibility.h Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/compatibility.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/sharded_store.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/handlers.cc Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/requests/compatibility.h Outdated Show resolved Hide resolved
tests/rptest/tests/schema_registry_test.py Outdated Show resolved Hide resolved
@pgellert
Copy link
Contributor Author

Force push:

  • First: address code review feedback
  • Second: fix linting error

@pgellert
Copy link
Contributor Author

Force-push: more code review feedback (conditionally rendering messages, changing transitional compat check order)

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Aug 16, 2024

BenPope
BenPope previously approved these changes Aug 19, 2024
Copy link
Member

@BenPope BenPope left a comment

Choose a reason for hiding this comment

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

LGTM

oleiman and others added 7 commits August 19, 2024 13:34
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
This matches the reference implementation and provides a more useful
output because the users likely prefer comparing the new schema against
the most recent incompatible schema.
@pgellert
Copy link
Contributor Author

Force-push: address rpk feedback + fix failing bazel build

@pgellert
Copy link
Contributor Author

@documentation @gene-redpanda @Deflaimun Let me know if you disagree but I believe your review won't be necessary for this PR because we are only extending the output of rpk registry schema check-compatibility with messages that match the reference implementation (here and here). There are no new rpk flags, rpk commands, redpanda configs or custom messages introduced.

Copy link
Contributor

@r-vasquez r-vasquez left a comment

Choose a reason for hiding this comment

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

rpk changes LGTM, thanks!

@pgellert pgellert merged commit 02ab333 into redpanda-data:dev Aug 20, 2024
25 checks passed
@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v24.2.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-22877-v24.2.x-170 remotes/upstream/v24.2.x
git cherry-pick -x 3771501a369f98f0e9a67dd8e28ee7b7bd047dc5 b27954caf6630fcae4b67b2fa3f2178069e88c81 25fcd5ef0c0807dcbb3b1ebc6043d7636358701c d8bcac608a37c396298957cef557e2716aa4efa1 992a8e7abd1667924f86fbe3b56ccf9afa97e0a9 2825e146adc42045cf54273ef4c447d81809ff16 1e31632169822c381d2276e4210e22af0262b1b4 dcf281c964911a34058215505946ee32d4c98305

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v24.1.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-22877-v24.1.x-554 remotes/upstream/v24.1.x
git cherry-pick -x 3771501a369f98f0e9a67dd8e28ee7b7bd047dc5 b27954caf6630fcae4b67b2fa3f2178069e88c81 25fcd5ef0c0807dcbb3b1ebc6043d7636358701c d8bcac608a37c396298957cef557e2716aa4efa1 992a8e7abd1667924f86fbe3b56ccf9afa97e0a9 2825e146adc42045cf54273ef4c447d81809ff16 1e31632169822c381d2276e4210e22af0262b1b4 dcf281c964911a34058215505946ee32d4c98305

Workflow run logs.

@vbotbuildovich
Copy link
Collaborator

Failed to create a backport PR to v23.3.x branch. I tried:

git remote add upstream https://github.com/redpanda-data/redpanda.git
git fetch --all
git checkout -b backport-pr-22877-v23.3.x-430 remotes/upstream/v23.3.x
git cherry-pick -x 3771501a369f98f0e9a67dd8e28ee7b7bd047dc5 b27954caf6630fcae4b67b2fa3f2178069e88c81 25fcd5ef0c0807dcbb3b1ebc6043d7636358701c d8bcac608a37c396298957cef557e2716aa4efa1 992a8e7abd1667924f86fbe3b56ccf9afa97e0a9 2825e146adc42045cf54273ef4c447d81809ff16 1e31632169822c381d2276e4210e22af0262b1b4 dcf281c964911a34058215505946ee32d4c98305

Workflow run logs.

@Deflaimun
Copy link
Contributor

@pgellert you're correct. I get tagged on every property and rpk change. When it's code only, and not doc strings, I just skip the PR.
Thanks!

*/
class avro_incompatibility {
public:
using Type = avro_incompatibility_type;
Copy link
Member

Choose a reason for hiding this comment

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

lowercase unless its a template parameter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants