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.

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

fix: do not overwrite schemas from a CREATE STREAM/TABLE #5756

Merged
merged 1 commit into from
Jul 1, 2020

Conversation

agavra
Copy link
Contributor

@agavra agavra commented Jul 1, 2020

Description

fixes #5553

This commit makes sure that C* statements do not overwrite existing schemas in schema registry and instead only check compatibility with a declared schema, if a declared schema is present.

NOTE: if you INSERT INTO a stream, all bets are off (i.e. another schema will be registered under the schema value, but that is not a regression as this is a serializer-specific behavior)

Testing done

Unit tests, and manual testing (registered the same source with a subset schema and confirmed there's only one version in schema registry)

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@agavra agavra marked this pull request as ready for review July 1, 2020 20:38
@agavra agavra requested a review from a team as a code owner July 1, 2020 20:38
@apurvam
Copy link
Contributor

apurvam commented Jul 1, 2020

Thanks almog. Do we know when the regression was introduced? Ie. which version of community ksqlDB?

@agavra
Copy link
Contributor Author

agavra commented Jul 1, 2020

@apurvam yes, the regression was introduced by #4717 which made it into 0.8.0

@@ -96,7 +99,8 @@ private void registerForCreateAs(final ConfiguredStatement<? extends CreateAsSel
queryMetadata.getResultTopic().getKafkaTopicName(),
queryMetadata.getResultTopic().getValueFormat().getFormatInfo(),
cas.getConfig(),
cas.getStatementText()
cas.getStatementText(),
true
Copy link
Member

Choose a reason for hiding this comment

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

Just to make sure, in which case to we want to overwrite existing schemas?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we want to overwrite existing schemas in the case of CREATE * AS SELECT

Copy link
Member

@vpapavas vpapavas left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants