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

[schema] Introduce schema data validator #4360

Merged
merged 14 commits into from
Jun 21, 2019
Merged

[schema] Introduce schema data validator #4360

merged 14 commits into from
Jun 21, 2019

Conversation

sijie
Copy link
Member

@sijie sijie commented May 24, 2019

Motivation

Currently the schema data is only validated in compatibility checker. If the schema data is uploaded from admin api, there is no validation if the schema is the first version.

Changes

Add schema data validator to validate the schema data before storing it in schema storage.

@sijie sijie added type/feature The PR added a new feature or issue requested a new feature component/schemaregistry labels May 24, 2019
@sijie sijie added this to the 2.4.0 milestone May 24, 2019
@sijie sijie self-assigned this May 24, 2019
codelipenghui
codelipenghui reviewed May 24, 2019
View reviewed changes
Copy link
Contributor

@codelipenghui codelipenghui left a comment

Choose a reason for hiding this comment

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

lgtm, just left a comment.

...ker/src/main/java/org/apache/pulsar/broker/service/schema/validator/SchemaDataValidator.java Outdated
validateSchemaData(kvSchema.getValue());
break;
default:
break;
Copy link
Contributor

Choose a reason for hiding this comment

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

is it better to throw a InvalidSchemaDataException? because use a unknown schema type.

Copy link
Member Author

Choose a reason for hiding this comment

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

good point. will change.

jiazhai
jiazhai approved these changes May 25, 2019
View reviewed changes
Copy link
Member

@jiazhai jiazhai left a comment

Choose a reason for hiding this comment

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

lgtm

@sijie
Copy link
Member Author

sijie commented May 26, 2019

@codelipenghui I have addressed your comment. PTAL.

@sijie
Copy link
Member Author

sijie commented May 26, 2019

run integration tests

congbobo184
congbobo184 approved these changes May 27, 2019
View reviewed changes
Copy link
Contributor

@congbobo184 congbobo184 left a comment

Choose a reason for hiding this comment

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

lgtm

@jiazhai
Copy link
Member

jiazhai commented Jun 1, 2019

run java8 tests

@sijie
Copy link
Member Author

sijie commented Jun 1, 2019

please don't retest this pull request. The cpp/python tests require some changes.

@sijie sijie modified the milestones: 2.4.0, 2.5.0 Jun 9, 2019
@sijie sijie mentioned this pull request Jun 20, 2019
Open
@sijie
Copy link
Member Author

sijie commented Jun 20, 2019

@jiazhai @codelipenghui @congbobo184 I pushed a fix (dfca5ae) to address python client issue apache/pulsar-client-python#30 . Please take a look.

@sijie
Copy link
Member Author

sijie commented Jun 20, 2019

run java8 tests

@codelipenghui
Copy link
Contributor

lgtm + 1

@codelipenghui codelipenghui modified the milestones: 2.5.0, 2.4.0 Jun 21, 2019
@congbobo184
Copy link
Contributor

lgtm + 1

@sijie sijie merged commit 6c13f18 into apache:master Jun 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jiazhai jiazhai jiazhai approved these changes

@codelipenghui codelipenghui codelipenghui approved these changes

@congbobo184 congbobo184 congbobo184 approved these changes

@merlimat merlimat Awaiting requested review from merlimat

Assignees

@sijie sijie

Labels
type/feature The PR added a new feature or issue requested a new feature
Projects
None yet
Milestone
2.4.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@sijie @jiazhai @codelipenghui @congbobo184