CORE-6860 Schema Registry: add detailed verbose compatibility checks #22958

pgellert · 2024-08-20T11:06:25Z

This implements detailed compatibility checks for protobuf and avro in schema registry. This enabled the schema registry and rpk to surface more specific information about the cause of the incompatibility between a new schema and the existing schemas by pointing the user to the exact incompatibility. This PR aims only to change the error reporting behaviour of the check compatibility endpoint when the ?verbose=true parameter is explicitly specified and does not change the rules of what schema pairs are (in)compatible.

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

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

Backports Required

Release Notes

Improvements

Schema Registry: now reports more specific error messages for Avro and Protobuf schemas when they are incompatible with earlier schemas.

Bug Fixes

Schema Registry: fixes a bug in the protobuf compatibility checks of nested fields where nested paths were incorrectly handled.

src/v/pandaproxy/schema_registry/avro.cc

oleiman

looks good. As noted, wrt avro aliases ahead of upstream support: I think we should err on the side of permissiveness, but I don't feel very strongly about it.

src/v/pandaproxy/schema_registry/avro.cc

src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc

pgellert · 2024-08-23T08:46:32Z

Force-push: minor cleanup after code review

vbotbuildovich · 2024-08-23T11:18:08Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/53437#01917eae-8e31-40a0-a7d7-e9c52dc6ba80

vbotbuildovich · 2024-08-23T11:24:38Z

new failures in https://buildkite.com/redpanda/redpanda/builds/53437#01917eae-8e35-4702-8ad0-06f37c1427a4:

"rptest.tests.rpk_registry_test.RpkRegistryTest.test_produce_consume_json"

new failures in https://buildkite.com/redpanda/redpanda/builds/53645#0191963a-9a6a-49ee-a8fd-012d2f6efd59:

"rptest.tests.scaling_up_test.ScalingUpTest.test_fast_node_addition"

BenPope

Looks great. Just a few minor things.

In general I find it easier to review code when it has tests, and tests when it has code, so I'd reorder and squash some commits.

Also, verbose out should be in the response of POST /subjects/<subject>/versions, which is roughly along the lines of:

index b882f9e003..01c79f85c9 100644
--- a/src/v/pandaproxy/schema_registry/sharded_store.cc
+++ b/src/v/pandaproxy/schema_registry/sharded_store.cc
@@ -201,14 +201,15 @@ sharded_store::get_schema_version(subject_schema schema) {
     // Check compatibility of the schema
     if (!v_id.has_value() && !versions.empty()) {
         auto compat = co_await is_compatible(
-          versions.back().version, schema.schema.share());
-        if (!compat) {
+          versions.back().version, schema.schema.share(), verbose::yes);
+        if (!compat.is_compat) {
             throw exception(
               error_code::schema_incompatible,
               fmt::format(
                 "Schema being registered is incompatible with an earlier "
-                "schema for subject \"{}\"",
-                sub));
+                "schema for subject \"{}\", details: {}",
+                sub,
+                compat.messages));
         }
     }
     co_return has_schema_result{s_id, v_id};

But the messages are rendered inside of "{}" instead of "[]"

src/v/pandaproxy/schema_registry/avro.cc

src/v/pandaproxy/schema_registry/test/compatibility_avro.cc

src/v/pandaproxy/schema_registry/test/compatibility_common.h

src/v/pandaproxy/schema_registry/protobuf.cc

BenPope · 2024-08-23T11:37:05Z

src/v/pandaproxy/schema_registry/protobuf.cc

+                compat_result.emplace<proto_incompatibility>(
+                  p / w->name(), proto_incompatibility::Type::message_removed);
+            } else {
+                compat_result.merge(check_compatible(r, w, p / w->name()));


nit: Missing test

Good spot! reader->FindNestedTypeByName above actually takes a relative name and not a fully qualified name

src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc

Common test code for working with detailed compatibility info about protobuf and avro schemas.

When an incompatible schema is posted, include the verbose compatibility messages in the error message. This helps users understand the reason for the incompatibility and it matches the reference implementation. Includes a minor fix to the way the compatibility level message is formatted.

pgellert · 2024-08-27T22:31:45Z

Force-push:

First: address code review feedback
Second: noop, reshuffling and squashing commits based on feedback

src/v/pandaproxy/schema_registry/compatibility.cc

oleiman

latest round lgtm

oleiman · 2024-08-27T22:58:41Z

src/v/pandaproxy/schema_registry/test/compatibility_avro.cc

+        BOOST_REQUIRE_MESSAGE(
+          errs == expected,
+          fmt::format("{} != {}", format_set(errs), format_set(expected)));
+    }


BenPope

LGTM

vbotbuildovich · 2024-08-28T14:44:50Z

/backport v24.2.x

vbotbuildovich · 2024-08-28T14:44:51Z

/backport v24.1.x

vbotbuildovich · 2024-08-28T14:44:51Z

/backport v23.3.x

vbotbuildovich · 2024-08-28T14:45:58Z

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-22958-v24.2.x-709 remotes/upstream/v24.2.x
git cherry-pick -x 11a87c3578ad396514bb8299bebeb5024ae37d1c ffecdd1a3f71256d08f5984d7590780cbfb95227 4c87c5ed21416d98f6d05a1c6c06ec0ab4949336 cda768b4a3da734f52ab04b5220c4d93b7e5fd97

Workflow run logs.

vbotbuildovich · 2024-08-28T14:46:03Z

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-22958-v23.3.x-412 remotes/upstream/v23.3.x
git cherry-pick -x 11a87c3578ad396514bb8299bebeb5024ae37d1c ffecdd1a3f71256d08f5984d7590780cbfb95227 4c87c5ed21416d98f6d05a1c6c06ec0ab4949336 cda768b4a3da734f52ab04b5220c4d93b7e5fd97

Workflow run logs.

vbotbuildovich · 2024-08-28T14:46:06Z

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-22958-v24.1.x-905 remotes/upstream/v24.1.x
git cherry-pick -x 11a87c3578ad396514bb8299bebeb5024ae37d1c ffecdd1a3f71256d08f5984d7590780cbfb95227 4c87c5ed21416d98f6d05a1c6c06ec0ab4949336 cda768b4a3da734f52ab04b5220c4d93b7e5fd97

Workflow run logs.

pgellert requested a review from a team August 20, 2024 11:06

pgellert self-assigned this Aug 20, 2024

pgellert requested review from michael-redpanda, BenPope and oleiman and removed request for a team August 20, 2024 11:06

github-actions bot added the area/redpanda label Aug 20, 2024

pgellert commented Aug 20, 2024

View reviewed changes

src/v/pandaproxy/schema_registry/avro.cc Show resolved Hide resolved

oleiman reviewed Aug 20, 2024

View reviewed changes

src/v/pandaproxy/schema_registry/avro.cc Show resolved Hide resolved

src/v/pandaproxy/schema_registry/test/compatibility_protobuf.cc Outdated Show resolved Hide resolved

pgellert force-pushed the sr/verbose-compat-3 branch from 54e856f to 9cab4da Compare August 23, 2024 08:45

pgellert requested a review from oleiman August 23, 2024 08:46

BenPope reviewed Aug 23, 2024

View reviewed changes

oleiman mentioned this pull request Aug 23, 2024

[CORE-1507] SR: Add support for ?verbose compatibility checks #18332

Closed

7 tasks

pgellert force-pushed the sr/verbose-compat-3 branch from 9cab4da to d9d29bb Compare August 27, 2024 22:27

oleiman and others added 4 commits August 27, 2024 23:28

sr/test: Introduce compatibility_common

11a87c3

Common test code for working with detailed compatibility info about protobuf and avro schemas.

sr/avro: Add detailed compatibility checks

ffecdd1

sr/protobuf: Add detailed compatibility checks

4c87c5e

pgellert force-pushed the sr/verbose-compat-3 branch from d9d29bb to cda768b Compare August 27, 2024 22:30

pgellert requested a review from BenPope August 27, 2024 22:33

oleiman reviewed Aug 27, 2024

View reviewed changes

src/v/pandaproxy/schema_registry/compatibility.cc Show resolved Hide resolved

oleiman reviewed Aug 27, 2024

View reviewed changes

BenPope approved these changes Aug 28, 2024

View reviewed changes

pgellert merged commit 2bd3b0d into redpanda-data:dev Aug 28, 2024
18 checks passed

vbotbuildovich mentioned this pull request Aug 28, 2024

[v24.2.x] CORE-6860 Schema Registry: add detailed verbose compatibility checks #23107

Closed

This was referenced Aug 28, 2024

[v23.3.x] CORE-6860 Schema Registry: add detailed verbose compatibility checks #23108

Closed

[v24.1.x] CORE-6860 Schema Registry: add detailed verbose compatibility checks #23109

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CORE-6860 Schema Registry: add detailed verbose compatibility checks #22958

CORE-6860 Schema Registry: add detailed verbose compatibility checks #22958

pgellert commented Aug 20, 2024 •

edited

Loading

oleiman left a comment

pgellert commented Aug 23, 2024

vbotbuildovich commented Aug 23, 2024

vbotbuildovich commented Aug 23, 2024 •

edited

Loading

BenPope left a comment

BenPope Aug 23, 2024

pgellert Aug 27, 2024

pgellert commented Aug 27, 2024

oleiman left a comment

oleiman Aug 27, 2024

BenPope left a comment

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

CORE-6860 Schema Registry: add detailed verbose compatibility checks #22958

CORE-6860 Schema Registry: add detailed verbose compatibility checks #22958

Conversation

pgellert commented Aug 20, 2024 • edited Loading

Backports Required

Release Notes

Improvements

Bug Fixes

oleiman left a comment

Choose a reason for hiding this comment

pgellert commented Aug 23, 2024

vbotbuildovich commented Aug 23, 2024

vbotbuildovich commented Aug 23, 2024 • edited Loading

BenPope left a comment

Choose a reason for hiding this comment

BenPope Aug 23, 2024

Choose a reason for hiding this comment

pgellert Aug 27, 2024

Choose a reason for hiding this comment

pgellert commented Aug 27, 2024

oleiman left a comment

Choose a reason for hiding this comment

oleiman Aug 27, 2024

Choose a reason for hiding this comment

BenPope left a comment

Choose a reason for hiding this comment

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

vbotbuildovich commented Aug 28, 2024

pgellert commented Aug 20, 2024 •

edited

Loading

vbotbuildovich commented Aug 23, 2024 •

edited

Loading