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-6824 Schema Registry/Proto: add 5 missing checks #22798

Merged
merged 5 commits into from
Aug 13, 2024
Merged

CORE-6824 Schema Registry/Proto: add 5 missing checks #22798

merged 5 commits into from
Aug 13, 2024

Conversation

pgellert
Copy link
Contributor

@pgellert pgellert commented Aug 8, 2024 •
edited
Loading

This implements 5 new compatibility checks for protobuf:

  • ONEOF_FIELD_REMOVED
  • MULTIPLE_FIELDS_MOVED_TO_ONEOF
  • REQUIRED_FIELD_{ADDED,REMOVED}
  • FIELD_NAMED_TYPE_CHANGED
  • MESSAGE_REMOVED

This gap was discovered and initially implemented on @oleiman's draft PR (#18332) aiming to add support for verbose logging. This PR extracts the changes of adding these new compatibility checks to make the review easier. Some unit tests are also added.

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

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

Improvements

  • Schema Registry: 5 new compatibility checks are added for protobuf (ONEOF_FIELD_REMOVED, MULTIPLE_FIELDS_MOVED_TO_ONEOF, REQUIRED_FIELD_{ADDED,REMOVED}, FIELD_NAMED_TYPE_CHANGED, MESSAGE_REMOVED)

@pgellert pgellert requested a review from a team August 8, 2024 14:32
@pgellert pgellert self-assigned this Aug 8, 2024
@pgellert pgellert requested review from aanthony-rp, BenPope and oleiman and removed request for a team August 8, 2024 14:32
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented Aug 8, 2024 •
edited
Loading

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52669#019132c3-608f-40b9-b13d-c0005b50a96a

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52799#019147ad-829e-4250-b8e3-da25c1dd1614

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/52799#019147ad-82a3-4c2d-8f2f-130c99438049

oleiman
oleiman reviewed Aug 8, 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.

looks good to me. leaving the approval status open since I wrote some of the code.

andijcr
andijcr previously approved these changes Aug 9, 2024
View reviewed changes
aanthony-rp
aanthony-rp previously approved these changes Aug 9, 2024
View reviewed changes
Copy link
Contributor

@aanthony-rp aanthony-rp left a comment

Choose a reason for hiding this comment

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

nothing standing out but I lack a lot of depth for context here. looks like oren and ben are digging in.

oleiman added 3 commits August 12, 2024 10:59
@oleiman @pgellert
sr/protobuf: Compatibility check: MESSAGE_REMOVED
1072f02 
Specifically we reed to enforce the presence of _nested_ message
types. Previously we only checked at file scope.

Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman @pgellert
sr/protobuf: Compatibility check: FIELD_NAMED_TYPE_CHANGED
bfc715b 
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@oleiman @pgellert
sr/protobuf: Compatibility check: REQUIRED_FIELD_{ADDED,REMOVED}
689ebad 
Signed-off-by: Oren Leiman <oren.leiman@redpanda.com>
@pgellert pgellert dismissed stale reviews from aanthony-rp and andijcr via 6dba17a August 12, 2024 10:51
@pgellert pgellert force-pushed the sr/missing-proto-checks branch from cfd74f7 to 6dba17a Compare August 12, 2024 10:51
@pgellert pgellert force-pushed the sr/missing-proto-checks branch from 6dba17a to d2e710a Compare August 12, 2024 11:26
@pgellert
Copy link
Contributor Author

Force-push (twice):

  • Removed reserved field checking logic
  • Test changes: added new tests, made the compatibility level specific for the added tests, structured all tests as a backward compatibility checks where the first line = new = reader schema and the second line = old = writer schema.

BenPope
BenPope reviewed Aug 12, 2024
View reviewed changes
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.

Very nice

@BenPope BenPope self-requested a review August 13, 2024 10:35
@pgellert pgellert merged commit 00c97fc into redpanda-data:dev Aug 13, 2024
20 checks passed
@pgellert pgellert deleted the sr/missing-proto-checks branch August 13, 2024 10:36
@vbotbuildovich
Copy link
Collaborator

/backport v24.2.x

@vbotbuildovich
Copy link
Collaborator

/backport v24.1.x

@vbotbuildovich
Copy link
Collaborator

/backport v23.3.x

This was referenced Aug 13, 2024
Merged
Merged
Merged
@oleiman oleiman mentioned this pull request Aug 23, 2024
Closed
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BenPope BenPope BenPope approved these changes

@andijcr andijcr andijcr approved these changes

@aanthony-rp aanthony-rp aanthony-rp left review comments

@oleiman oleiman Awaiting requested review from oleiman

Assignees

@pgellert pgellert

Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pgellert @vbotbuildovich @andijcr @BenPope @oleiman @aanthony-rp