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

bulk-cdk-toolkit-extract-cdc: fix CustomConverter bug #50967

Merged
merged 1 commit into from
Jan 7, 2025

Conversation

postamar
Copy link
Contributor

@postamar postamar commented Jan 7, 2025

What

I made a mistake in assuming that the outputSchema value could be re-used for multiple columns, but that turns out to not be the case. What debezium really wants is a new instance of a SchemaBuilder for each column for which we register a CustomConverter.

This bug becomes apparent when applying the same converter logic to two columns of the same type in the same table and with default values. This isn't covered in any test and I'm not sure that this can be tested adequately in the CDK.

How

This requires turning outputSchema from a value to a function, at which point I felt it made more sense to turn Handler into an interface.

Review guide

See accompanying PR #50965 which applies these changes to source-mysql.

User Impact

Should unblock the source-mysql rollout.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@postamar postamar requested a review from a team as a code owner January 7, 2025 18:59
Copy link

vercel bot commented Jan 7, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 7, 2025 7:05pm

@rodireich
Copy link
Contributor

rodireich commented Jan 7, 2025

If you make the change here to reinstantiate a SchemaBuilder, why is there any change needed for mysql? i.e #50965

@postamar
Copy link
Contributor Author

postamar commented Jan 7, 2025

I change the Handler from a data class to an interface. 2 out of 3 of its args were lambdas at this point, also it removes a few layers of indentation in the implementations.

@postamar postamar merged commit 53ec282 into master Jan 7, 2025
26 checks passed
@postamar postamar deleted the postamar/bulk-cdk-fix-converter-bug branch January 7, 2025 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants