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

feat(kafka): support several schemas #32

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leo-dur
Copy link
Contributor

@leo-dur leo-dur commented Feb 8, 2024

Changes

  • Add support for several schemas

Why several schemas support is needed?

On kafka-stream repos, we have 0-n output topics and 0-n internal topics (to record state).
Each topics usually need 1 or 2 schemas (either only value, or both key and value)

Note: Internal topics are read and written by the same app.

Example

https://github.com/Kpler/static-serializer-stream/pull/105/files

On that repo we have 2 output topics:

  • installation_document
  • berth_document

We have one schema per topic for the value (so 2 schemas)

We also have 1 internal topic with a schema for the value and the key

@leo-dur leo-dur self-assigned this Feb 8, 2024
@@ -43,14 +43,16 @@ find_schema_class_file() {
# Otherwise we fallback on finding the filename containing the schema code
# to end with Schema or is named InputModel
# We might want to improve this in the future
schema_class_file="$(grep -lr "^@schema" src | head -n 1 || return 0)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

take only one

@@ -43,14 +43,16 @@ find_schema_class_file() {
# Otherwise we fallback on finding the filename containing the schema code
# to end with Schema or is named InputModel
# We might want to improve this in the future
schema_class_file="$(grep -lr "^@schema" src | head -n 1 || return 0)"
schema_class_files="$(grep -lr "^@schema" src || return 0)"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

take all

@leo-dur leo-dur force-pushed the kafka-hooks-support-several-schema branch from 80adf35 to a7f9367 Compare February 8, 2024 05:50
@leo-dur leo-dur requested a review from yannrouillard February 8, 2024 16:04
Copy link
Contributor

@yannrouillard yannrouillard left a comment

Choose a reason for hiding this comment

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

I did some minor change + I kept the previous behavior of using only one schemas.avsc file when there is only one schema.

We can re-discuss that change but we need to take into account the impact on the deployment: currently, at deployment time the only schema to upload to the schema registry is simply found by checking for schema-.avsc.

If we stop using that, either the schema must be always explicitly specified for each topic in the values file or the logic should be improved to check how many schema file are available and to pick the only existing one when there is only one available and only one topic configured.

We also need to update the 2 actions https://github.com/Kpler/github-actions/blob/main/actions/kafka/upload-kafka-schemas/ and https://github.com/Kpler/github-actions/blob/main/actions/kafka/check-kafka-schemas-compatibility/ to support the multiple schema case (shouldn't be very complicated for these two).

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.

2 participants