Schema Registry: `GET /schemas/ids/<id>` should return references #11216

BenPope · 2023-06-05T22:41:47Z

Return references for GET /schemas/ids/<id>.

Notes for reviewer:

The store has two mappings:

schema_map: Schema ID to Schema
subject_map: Subject-Versions to Schema ID

Each of these mappings are then balanced across shards, coordinated by sharded_store.

Prior to this change, references were attached to the Subject-Versions.

This is an incorrect modelling; Schema should own the references to other schema. This commit does that refactoring by moving references from typed_schema to typed_schema_definition.

This allows returning the references when querying a schema by ID.

Additionally there is a fix to ignore references when a subject-version has been soft-deleted.

Backports Required

Release Notes

Bug Fixes

Schema Registry: Return references for GET /schemas/ids/<id>.

This will make the next refactor slightly easier. Signed-off-by: Ben Pope <ben@redpanda.com>

This will make the next refactor slightly easier to comprehend. Signed-off-by: Ben Pope <ben@redpanda.com>

Signed-off-by: Ben Pope <ben@redpanda.com>

dotnwat · 2023-06-06T04:09:23Z

src/v/pandaproxy/schema_registry/sharded_store.cc

+          acc.insert(acc.end(), refs.begin(), refs.end());
+          return acc;
+      });
+    absl::c_sort(references);


is htis different than std::sort used above?

Fixed by using absl::btree_set

dotnwat · 2023-06-06T04:13:43Z

src/v/pandaproxy/schema_registry/store.h

+    std::vector<schema_id>
+    subject_versions_with_any_of(const std::vector<schema_id>& ids) {
+        std::vector<schema_id> has_ids;
+        for (const auto& s : _subjects) {
+            for (const auto& r : s.second.versions) {
+                if (absl::c_binary_search(ids, r.id)) {
+                    has_ids.push_back(r.id);
+                }
+            }
+        }
+        return has_ids;
+    }
+
+    bool subject_versions_has_any_of(const std::vector<schema_id>& ids) {
+        return absl::c_any_of(_subjects, [&ids](const auto& s) {
+            return absl::c_any_of(s.second.versions, [&ids](const auto& v) {
+                return absl::c_binary_search(ids, v.id);
+            });
+        });
+    }
+


i assume these require the inputs to be sorted? probably worth a comment

Fixed by using absl::btree_set

This should be a pure refactoring. The store has two mappings: 1) `schema_map`: Schema ID to Schema (sharded across cores) 2) `subject_map`: Subject-Versions to Schema ID Each of these mappings are then split across shards, coordinated by sharded_store. Prior to this change, references were attached to the Subject-Versions. This is an incorrect modelling; Schema should own the references to other schema. This commit does that refactoring by moving references from `typed_schema` to `typed_schema_definition`. There are a couple of things that are not straightforward: 1) `ValidSchema` (`protobuf_schema_definition`, `avro_schema_definition`) are the respective library instantiations of the representation, and are unable to reproduce their references to form a `canonical_schema_definition`, so references are stored inside the wrappers. 2) `referenced_by` and `is_reference` now require two phases to obtain the references, due to the sharded and diconnected nature of the two mappings (schema, and subject-versions). a) Obtain a list of schema ids that reference the subject-version b) Filter that list by subject-versions that are not soft-deleted Signed-off-by: Ben Pope <ben@redpanda.com>

Signed-off-by: Ben Pope <ben@redpanda.com>

`GET /schemas/ids/<id>` now returns its references. Fixes redpanda-data#11194 Signed-off-by: Ben Pope <ben@redpanda.com>

BenPope · 2023-06-06T11:12:35Z

Changes in force-push:

Refactor the store references code by using an absl::btree_set to simplify the logic and enforce the constraints.

BenPope · 2023-06-06T14:28:08Z

CI Failures:

dotnwat

lgtm.

just to confirm:

    schema_id_set referenced_by(const subject& sub, schema_version ver) {
        schema_id_set references;
        for (const auto& s : _schemas) {
            for (const auto& r : s.second.definition.refs()) {
                if (r.sub == sub && r.version == ver) {
                    references.emplace_back(s.first);
                    references.insert(s.first);
                }
            }
        }

The referenced_by helper doesn't need to integrate the soft-delete flag?

BenPope · 2023-06-06T18:29:29Z

The referenced_by helper doesn't need to integrate the soft-delete flag?

Good question. I poked around and found no mechanism for it to be required.

vbotbuildovich · 2023-06-06T18:38:02Z

/backport v23.1.x

vbotbuildovich · 2023-06-06T18:38:03Z

/backport v22.3.x

vbotbuildovich · 2023-06-06T18:38:04Z

/backport v22.2.x

vbotbuildovich · 2023-06-06T18:38:52Z

Failed to run cherry-pick command. I executed the commands below:

git checkout -b backport-pr-11216-v23.1.x-210 remotes/upstream/v23.1.x
git cherry-pick -x f9178757c31231ab2fa3896e8a9ac13a556a3c2f 4972bcc7f33802561db7ea44df077e7cbcf6f705 5d5072bd79603128768841b3dba98aa4e5b54dbd 2fc2dfee3b14b19ecc729c80d2b52bf344a7de73 c13d874f464da1b7d1981c0cdb626b7256986a76 92987882a32cdd6221e20a58d43092890fa5a178

Workflow run logs.

vbotbuildovich · 2023-06-06T18:38:53Z

Failed to run cherry-pick command. I executed the commands below:

git checkout -b backport-pr-11216-v22.2.x-354 remotes/upstream/v22.2.x
git cherry-pick -x f9178757c31231ab2fa3896e8a9ac13a556a3c2f 4972bcc7f33802561db7ea44df077e7cbcf6f705 5d5072bd79603128768841b3dba98aa4e5b54dbd 2fc2dfee3b14b19ecc729c80d2b52bf344a7de73 c13d874f464da1b7d1981c0cdb626b7256986a76 92987882a32cdd6221e20a58d43092890fa5a178

Workflow run logs.

vbotbuildovich · 2023-06-06T18:38:56Z

Failed to run cherry-pick command. I executed the commands below:

git checkout -b backport-pr-11216-v22.3.x-596 remotes/upstream/v22.3.x
git cherry-pick -x f9178757c31231ab2fa3896e8a9ac13a556a3c2f 4972bcc7f33802561db7ea44df077e7cbcf6f705 5d5072bd79603128768841b3dba98aa4e5b54dbd 2fc2dfee3b14b19ecc729c80d2b52bf344a7de73 c13d874f464da1b7d1981c0cdb626b7256986a76 92987882a32cdd6221e20a58d43092890fa5a178

Workflow run logs.

BenPope added 3 commits June 5, 2023 23:02

schema_registry/types: Refector: Shuffle some types around

f917875

This will make the next refactor slightly easier. Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/test: Refactor: Shuffle some definitions around

4972bcc

This will make the next refactor slightly easier to comprehend. Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/test: Add more tests for references

5d5072b

Signed-off-by: Ben Pope <ben@redpanda.com>

BenPope self-assigned this Jun 5, 2023

github-actions bot added the area/redpanda label Jun 5, 2023

BenPope added area/schema-registry Schema Registry service within Redpanda and removed area/redpanda labels Jun 5, 2023

BenPope force-pushed the schema_registr_schema_by_id_with_references branch from a42bfb9 to 7a03737 Compare June 5, 2023 22:42

github-actions bot added the area/redpanda label Jun 5, 2023

BenPope force-pushed the schema_registr_schema_by_id_with_references branch from 7a03737 to a408202 Compare June 5, 2023 22:46

BenPope requested review from dotnwat and michael-redpanda June 5, 2023 22:46

BenPope force-pushed the schema_registr_schema_by_id_with_references branch from a408202 to 05de998 Compare June 5, 2023 23:54

dotnwat previously approved these changes Jun 6, 2023

View reviewed changes

BenPope added 3 commits June 6, 2023 12:08

schema_registry/store: Soft-deleted sub-ver are not references

c13d874

Signed-off-by: Ben Pope <ben@redpanda.com>

schema_registry/store: get_schema now returns references

9298788

`GET /schemas/ids/<id>` now returns its references. Fixes redpanda-data#11194 Signed-off-by: Ben Pope <ben@redpanda.com>

BenPope dismissed dotnwat’s stale review via 9298788 June 6, 2023 11:10

BenPope force-pushed the schema_registr_schema_by_id_with_references branch from 05de998 to 9298788 Compare June 6, 2023 11:10

BenPope requested a review from dotnwat June 6, 2023 17:38

dotnwat approved these changes Jun 6, 2023

View reviewed changes

dotnwat merged commit 82ed4e4 into redpanda-data:dev Jun 6, 2023

vbotbuildovich mentioned this pull request Jun 6, 2023

[v23.1.x] Schema Registry: GET /schemas/ids/<id> should return references #11237

Closed

This was referenced Jun 6, 2023

[v22.2.x] Schema Registry: GET /schemas/ids/<id> should return references #11238

Closed

[v22.3.x] Schema Registry: GET /schemas/ids/<id> should return references #11239

Closed

This was referenced Jun 6, 2023

[v23.1.x] Schema Registry: GET /schemas/ids/<id> should return references #11240

Merged

[v22.3.x] Schema Registry: GET /schemas/ids/<id> should return references #11241

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Schema Registry: `GET /schemas/ids/<id>` should return references #11216

Schema Registry: `GET /schemas/ids/<id>` should return references #11216

BenPope commented Jun 5, 2023 •

edited

Loading

dotnwat Jun 6, 2023

BenPope Jun 6, 2023

dotnwat Jun 6, 2023

BenPope Jun 6, 2023

BenPope commented Jun 6, 2023

BenPope commented Jun 6, 2023

dotnwat left a comment

BenPope commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

Schema Registry: GET /schemas/ids/<id> should return references #11216

Schema Registry: GET /schemas/ids/<id> should return references #11216

Conversation

BenPope commented Jun 5, 2023 • edited Loading

Notes for reviewer:

Backports Required

Release Notes

Bug Fixes

dotnwat Jun 6, 2023

Choose a reason for hiding this comment

BenPope Jun 6, 2023

Choose a reason for hiding this comment

dotnwat Jun 6, 2023

Choose a reason for hiding this comment

BenPope Jun 6, 2023

Choose a reason for hiding this comment

BenPope commented Jun 6, 2023

BenPope commented Jun 6, 2023

dotnwat left a comment

Choose a reason for hiding this comment

BenPope commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

vbotbuildovich commented Jun 6, 2023

Schema Registry: `GET /schemas/ids/<id>` should return references #11216

Schema Registry: `GET /schemas/ids/<id>` should return references #11216

BenPope commented Jun 5, 2023 •

edited

Loading