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.

Sign up for GitHub

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

Jump to bottom

[CORE-3181] Schema Registry: Normalization #22519

Merged
merged 9 commits into from
Jul 26, 2024
Merged

[CORE-3181] Schema Registry: Normalization #22519

merged 9 commits into from
Jul 26, 2024

Conversation

BenPope
Copy link
Member

@BenPope BenPope commented Jul 26, 2024

TODO: Add some more tests

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: Support normalize=true

Improvements

  • Schema Registry: Improve AVRO Normalization

@BenPope BenPope added the area/schema-registry Schema Registry service within Redpanda label Jul 26, 2024
@BenPope BenPope added this to the v24.2.1-final-RC milestone Jul 26, 2024
@BenPope BenPope requested a review from a team July 26, 2024 10:18
@BenPope BenPope self-assigned this Jul 26, 2024
@BenPope BenPope requested a review from a team as a code owner July 26, 2024 10:18
@BenPope BenPope requested review from oleiman and removed request for a team July 26, 2024 10:18
@BenPope BenPope requested a review from andijcr July 26, 2024 10:18
andijcr
andijcr reviewed Jul 26, 2024
View reviewed changes
Copy link
Contributor

@andijcr andijcr left a comment

Choose a reason for hiding this comment

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

looks good so far

src/v/pandaproxy/schema_registry/types.h Show resolved Hide resolved
tests/rptest/tests/schema_registry_test.py Outdated Show resolved Hide resolved
src/v/pandaproxy/schema_registry/json.cc
Comment on lines +1444 to +1485
case rapidjson::Type::kArrayType: {
for (auto& v : val.GetArray()) {
sort(v);
}
break;
}
Copy link
Contributor

@andijcr andijcr Jul 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

some arrays should be sorted i think. looking at draft7

        "allOf": { "$ref": "#/definitions/schemaArray" },
        "anyOf": { "$ref": "#/definitions/schemaArray" },
        "oneOf": { "$ref": "#/definitions/schemaArray" },
        "required": { "$ref": "#/definitions/stringArray" },
        "enum": {
            "type": "array",
            "items": true,
            "minItems": 1,
            "uniqueItems": true
        },
        "type": {
            "anyOf": [
                { "$ref": "#/definitions/simpleTypes" },
                {
                    "type": "array",
                    "items": { "$ref": "#/definitions/simpleTypes" },
                    "minItems": 1,
                    "uniqueItems": true
                }
            ]
        },

(note that "items" should not be sorted)

Copy link
Member Author

Choose a reason for hiding this comment

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

This'll have to wait.

@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Jul 26, 2024 •
edited
Loading

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52086#0190eed2-6772-4185-b3fd-3bb763a7713b

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52086#0190eed2-6773-499f-bee7-1c759a843069

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52086#0190eed2-6770-41c7-b35f-c79a469a5ffc

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52119#0190f09c-41ff-48c1-88e7-22bde8a3c5ce

pgellert
pgellert reviewed Jul 26, 2024
View reviewed changes
src/v/pandaproxy/schema_registry/sharded_store.cc Show resolved Hide resolved
src/v/pandaproxy/schema_registry/handlers.cc Show resolved Hide resolved
tests/rptest/tests/schema_registry_test.py
result_raw = self._get_schemas_ids_id(id=v1_id)
assert result_raw.status_code == requests.codes.ok
assert result_raw.json()['schema'] == dataset.schema_canonical

Copy link
Contributor

Choose a reason for hiding this comment

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

If you rebase to dev, I've added a test with a TODO for the confluent SR client for normalization that might be nice to uncomment/implement now: https://github.com/redpanda-data/redpanda/pull/22328/files

Copy link
Member Author

Choose a reason for hiding this comment

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

This will have to wait

BenPope added 5 commits July 26, 2024 18:08
@BenPope
pandaproxy/parsing: More lenient bool parsing
6621613 
Specifically, accept `True` by using case-insensitive comparison.

Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry: Allow sorting references
7a9fcfa 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry/store: Allow normalization
26dfa33 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry: Allow normalization at startup
a2a24d7 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry: Allow normalization over REST
fabfbc5 
Signed-off-by: Ben Pope <ben@redpanda.com>
oleiman
oleiman reviewed Jul 26, 2024
View reviewed changes
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 so far

src/v/pandaproxy/api/api-doc/schema_registry.json
Comment on lines +796 to +797
"type": "boolean"
},
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: I think we're not in the habit of this generally, but can we include the default value here?

@BenPope BenPope force-pushed the schema_registry/normalize_schema branch from 280bc06 to fb4f5f0 Compare July 26, 2024 17:47
BenPope added 3 commits July 26, 2024 18:48
@BenPope
schema_registry/avro: Fix recursion for normalization
0772f65 
Fixes CORE-5704

TODO: Add tests

Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry/dt: Refactor
f06cbaf 
No functional changes

Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope
schema_registry/dt: Add tests for normalization
86feb13 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope BenPope force-pushed the schema_registry/normalize_schema branch from fb4f5f0 to b0058a1 Compare July 26, 2024 17:52
@BenPope BenPope requested review from andijcr and oleiman July 26, 2024 17:53
@BenPope
Copy link
Member Author

BenPope commented Jul 26, 2024

Changes in force-push

  • rebase

Changes in force-push

  • More tests

@BenPope BenPope requested a review from pgellert July 26, 2024 17:56
@BenPope
schema_registry: Allow normalization of JSON Schema
a453390 
Signed-off-by: Ben Pope <ben@redpanda.com>
@BenPope BenPope force-pushed the schema_registry/normalize_schema branch from b0058a1 to a453390 Compare July 26, 2024 18:15
oleiman
oleiman approved these changes Jul 26, 2024
View reviewed changes
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

@BenPope BenPope merged commit 0f4000b into redpanda-data:dev Jul 26, 2024
20 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 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-22519-v23.3.x-120 remotes/upstream/v23.3.x
git cherry-pick -x 66216133c351c2cb32955a7c2a93824249d2ec19 7a9fcfa8b2260d4b6542bb3d8e0553a2bf97c02f 26dfa333d7057cc088968bde314b6293e32db3b1 a2a24d7d243a08c76f5e8c128802aefe7185ee12 fabfbc5ae69ebbf90ebd354d5f262ec8ad71a429 0772f65c063fe0a541ff2bdb73859c574371b9c3 f06cbaf63d501a2a3d42b7ea3797812f6188edd7 86feb1392a1a99a138f7f2bb7fd6d72d3c8e33e9 a4533901edc18a8ade573403500fb984be6ea34b

Workflow run logs.

@vbotbuildovich vbotbuildovich mentioned this pull request Jul 26, 2024
Closed
@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-22519-v24.2.x-193 remotes/upstream/v24.2.x
git cherry-pick -x 66216133c351c2cb32955a7c2a93824249d2ec19 7a9fcfa8b2260d4b6542bb3d8e0553a2bf97c02f 26dfa333d7057cc088968bde314b6293e32db3b1 a2a24d7d243a08c76f5e8c128802aefe7185ee12 fabfbc5ae69ebbf90ebd354d5f262ec8ad71a429 0772f65c063fe0a541ff2bdb73859c574371b9c3 f06cbaf63d501a2a3d42b7ea3797812f6188edd7 86feb1392a1a99a138f7f2bb7fd6d72d3c8e33e9 a4533901edc18a8ade573403500fb984be6ea34b

Workflow run logs.

@vbotbuildovich vbotbuildovich mentioned this pull request Jul 26, 2024
Closed
@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-22519-v24.1.x-412 remotes/upstream/v24.1.x
git cherry-pick -x 66216133c351c2cb32955a7c2a93824249d2ec19 7a9fcfa8b2260d4b6542bb3d8e0553a2bf97c02f 26dfa333d7057cc088968bde314b6293e32db3b1 a2a24d7d243a08c76f5e8c128802aefe7185ee12 fabfbc5ae69ebbf90ebd354d5f262ec8ad71a429 0772f65c063fe0a541ff2bdb73859c574371b9c3 f06cbaf63d501a2a3d42b7ea3797812f6188edd7 86feb1392a1a99a138f7f2bb7fd6d72d3c8e33e9 a4533901edc18a8ade573403500fb984be6ea34b

Workflow run logs.

@vbotbuildovich vbotbuildovich mentioned this pull request Jul 26, 2024
Closed
@BenPope BenPope removed this from the v24.2.1-final-RC milestone Jul 26, 2024
@BenPope
Copy link
Member Author

BenPope commented Jul 26, 2024

/backport v24.2.x

@vbotbuildovich vbotbuildovich mentioned this pull request Jul 26, 2024
Merged
This was referenced Jul 29, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oleiman oleiman oleiman approved these changes

@andijcr andijcr andijcr approved these changes

@pgellert pgellert Awaiting requested review from pgellert

Assignees

@BenPope BenPope

Labels
area/redpanda area/schema-registry Schema Registry service within Redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@BenPope @vbotbuildovich @andijcr @oleiman @pgellert