refactor: introduce new api to control schema translation #8309

lihaosky · 2021-11-02T20:46:07Z

Description

Remove key_schema_id and value_schema_id from ConnectFormat.
Introduce new api in Format to control how schema should be translated.
Keep current behavior (uppercase field names) for Schema Inference without providing schema id.
Remove key_schema_id and value_schema_id from format properties.
Remove historical_plans previously added as a result of schema_id in format properties.

Testing done

Update unit test

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 #")

vcrfxia

Thanks @lihaosky ! This refactor LGTM but I have a question about how it fits into the larger KLIP-56 project: once this PR is merged, it will be the case that if key_schema_id/value_schema_id is provided, then ksqlDB will no longer uppercase fields in the schema read from Schema Registry, right? Doesn't that mean this PR should be accompanied by changes to the QTTs which explicitly set key_schema_id or value_schema_id to expect lowercase field names, instead of uppercase? How come I don't see those changes?

vcrfxia · 2021-11-03T21:44:17Z

Also one more question: I know there are more PRs planned for KLIP-56. Are we OK with being in a partial state for this project when the 0.24 release is cut? It doesn't look like development on this feature is happening behind a feature flag.

lihaosky · 2021-11-03T21:51:14Z

Good question! I'm updating the tests now.

What's general way of put this behind feature flag? Adding key_schema_id and value_schema_id at last in property to make it available? BTW, key_schema_id and value_schema_id is already available for CS/CT before KLIP-56. KLIP-56 introduce this case fix and other validations. I was under the impression that updating documentation makes this available to use publicly.

vcrfxia · 2021-11-03T22:44:41Z

For larger features, we usually introduce a feature flag into KsqlConfig at the start of feature development and hide all functional changes behind the feature flag so it's safe to release at any point during feature development (even if the feature is only half-baked). In your particular case, I agree with your assessment: I think it's fine that we're in a half-baked state with this feature for 0.24 since the WITH clause properties already existed and were undocumented; I'm not aware of any users who are relying on the feature (though if that understanding is wrong then we might want to rethink this). I just wanted to double-check that we're not in a state where development of this feature impacts other ksql behaviors (e.g., if the previous version of this PR had been merged, then existing schema inference would've resulted in field case being preserved, which would have been a visible change).

vcrfxia

Thanks @lihaosky -- LGTM besides the minor comment I left inline, but I notice that the build is also failing. The current failures seem to be checkstyle related. It'd be good to unblock those in case it's hiding test failures.

Also, it'd be good to merge this PR soon in order to block the 0.24 release cut. I'd rather not cut the release without this PR being merged since then we'd be writing key_schema_id and value_schema_id into the command topic as part of a released version, and we'd have to maintain backwards compatibility for such command topic entries. Unless you think this PR will be ready to merge by the end of tomorrow (Friday), can you open a separate PR just to back out the changes that are causing key_schema_id and value_schema_id to be written into the command topic, in order to unblock the release? Thanks!

vcrfxia · 2021-11-04T21:27:09Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/elements.json

+    {
+      "name": "validate schema id without elements OK - PROTOBUF",
+      "statements": [
+        "CREATE STREAM INPUT WITH (kafka_topic='input', value_format='PROTOBUF');",


I don't see schema id in this statement?

Good catch!

lihaosky

Thanks @vcrfxia ! Should be able to merge this one by end of tomorrow.

lihaosky · 2021-11-05T01:22:41Z

ksqldb-functional-tests/src/test/resources/query-validation-tests/elements.json

+    {
+      "name": "validate schema id without elements OK - PROTOBUF",
+      "statements": [
+        "CREATE STREAM INPUT WITH (kafka_topic='input', value_format='PROTOBUF');",


Good catch!

vcrfxia

Thanks @lihaosky -- LGTM besides the comments inline (and the failing build).

vcrfxia · 2021-11-05T16:07:58Z

ksqldb-functional-tests/src/test/resources/sql-tests/test.sql

@@ -57,7 +57,7 @@ ASSERT VALUES bar (rowtime, id, col1) VALUES (1, 1, 1);
 --@test: basic test with key_schema_id (format AVRO)
 ----------------------------------------------------------------------------------------------------
 CREATE STREAM foo (id INT KEY, col1 INT) WITH (kafka_topic='foo', key_format='AVRO', value_format='AVRO', key_schema_id=123);
-ASSERT STREAM foo (id INT KEY, col1 INT) WITH (kafka_topic='foo', key_format='AVRO', value_format='AVRO', key_schema_id=123);


Why is this being removed? The test is called basic test with key_schema_id.

It was added by my previos PR when putting schema id to format properties: 84e1765. Not we don't put it there, it's deleted

Then we should delete the entire test, right? The test currently no longer tests what the name of the test suggests.

vcrfxia · 2021-11-05T16:08:20Z

ksqldb-functional-tests/src/test/resources/sql-tests/test.sql

-ASSERT STREAM bar (id INT KEY, col1 INT) WITH (kafka_topic='BAR', value_format='JSON', wrap_single_value=false);
-
----------------------------------------------------------------------------------------------------
--@test: assert format doesn't support schema_id property should fail


Why is this test being removed?

When we previously put schema id to format properties. We expect KAFKA not support it and throw exception, now we don't put it there, it will be ignored. It was added in 84e1765. I have next PR to do validation so that we throw exceptions when schema id is provided and format doesn't support schema inference

lihaosky

Let me take a look at build again...

lihaosky · 2021-11-05T17:00:10Z

ksqldb-functional-tests/src/test/resources/sql-tests/test.sql

@@ -57,7 +57,7 @@ ASSERT VALUES bar (rowtime, id, col1) VALUES (1, 1, 1);
 --@test: basic test with key_schema_id (format AVRO)
 ----------------------------------------------------------------------------------------------------
 CREATE STREAM foo (id INT KEY, col1 INT) WITH (kafka_topic='foo', key_format='AVRO', value_format='AVRO', key_schema_id=123);
-ASSERT STREAM foo (id INT KEY, col1 INT) WITH (kafka_topic='foo', key_format='AVRO', value_format='AVRO', key_schema_id=123);


It was added by my previos PR when putting schema id to format properties: 84e1765. Not we don't put it there, it's deleted

lihaosky · 2021-11-05T17:02:06Z

ksqldb-functional-tests/src/test/resources/sql-tests/test.sql

-ASSERT STREAM bar (id INT KEY, col1 INT) WITH (kafka_topic='BAR', value_format='JSON', wrap_single_value=false);
-
----------------------------------------------------------------------------------------------------
--@test: assert format doesn't support schema_id property should fail


When we previously put schema id to format properties. We expect KAFKA not support it and throw exception, now we don't put it there, it will be ignored. It was added in 84e1765. I have next PR to do validation so that we throw exceptions when schema id is provided and format doesn't support schema inference

vcrfxia

LGTM once the build passes. Thanks @lihaosky !

refactor: introduce new api to control schema translation

41388ff

lihaosky requested a review from vcrfxia November 2, 2021 20:46

lihaosky requested a review from a team as a code owner November 2, 2021 20:46

lihaosky added 2 commits November 2, 2021 15:41

fix: remove schema_id from format properties

192427c

fix: fix checkstyle

aac5c2a

vcrfxia reviewed Nov 3, 2021

View reviewed changes

lihaosky added 2 commits November 3, 2021 23:13

test: add QTT for schema id

8bb88da

test: regen plan for new QTT test

bf6bb15

vcrfxia reviewed Nov 4, 2021

View reviewed changes

fix: fix test and checkstyle

694d44b

lihaosky commented Nov 5, 2021

View reviewed changes

fix: fix test

9455c58

lihaosky force-pushed the refactor-translation branch from 315c4a6 to 9455c58 Compare November 5, 2021 06:08

vcrfxia reviewed Nov 5, 2021

View reviewed changes

lihaosky commented Nov 5, 2021

View reviewed changes

lihaosky added 2 commits November 5, 2021 10:12

fix: regen QTT test

434f8d3

test: remove schema id test in test.sql

e9c7404

vcrfxia approved these changes Nov 5, 2021

View reviewed changes

lihaosky merged commit 6913cf6 into confluentinc:master Nov 5, 2021

lihaosky deleted the refactor-translation branch November 5, 2021 22:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: introduce new api to control schema translation #8309

refactor: introduce new api to control schema translation #8309

lihaosky commented Nov 2, 2021 •

edited

vcrfxia left a comment

vcrfxia commented Nov 3, 2021

lihaosky commented Nov 3, 2021

vcrfxia commented Nov 3, 2021

vcrfxia left a comment

vcrfxia Nov 4, 2021

lihaosky Nov 5, 2021

lihaosky left a comment

lihaosky Nov 5, 2021

vcrfxia left a comment

vcrfxia Nov 5, 2021

lihaosky Nov 5, 2021

vcrfxia Nov 5, 2021

vcrfxia Nov 5, 2021

lihaosky Nov 5, 2021

lihaosky left a comment

lihaosky Nov 5, 2021

lihaosky Nov 5, 2021

vcrfxia left a comment

refactor: introduce new api to control schema translation #8309

refactor: introduce new api to control schema translation #8309

Conversation

lihaosky commented Nov 2, 2021 • edited

Description

Testing done

Reviewer checklist

vcrfxia left a comment

Choose a reason for hiding this comment

vcrfxia commented Nov 3, 2021

lihaosky commented Nov 3, 2021

vcrfxia commented Nov 3, 2021

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaosky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lihaosky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcrfxia left a comment

Choose a reason for hiding this comment

lihaosky commented Nov 2, 2021 •

edited