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

MSSQL and debezium improvements #44759

Conversation

rodireich
Copy link
Contributor

@rodireich rodireich commented Aug 26, 2024

What

A few issues in debezium and mssql were discovered as a result of https://github.com/airbytehq/oncall/issues/6192 .

  1. Conversion of incoming debezium change events to ChangeEventWithMetadata was not null safe.
  2. During an incremental run of mssql, we'd run debezium with configuration that could turn into initial snapshot.
  3. We processed incoming debezium events categorized as op: r which is can only be part of an initial snapshot.

This PR deals with the discoveries and adds a new troubleshooting page for the root cause of the oncall issue.

Copy link

vercel bot commented Aug 26, 2024

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 Aug 28, 2024 10:53pm

@octavia-squidington-iii octavia-squidington-iii added the CDK Connector Development Kit label Aug 26, 2024
@@ -181,7 +181,7 @@ class DebeziumRecordIterator<T>(
hasSnapshotFinished = !changeEventWithMetadata.isSnapshotEvent

if (isEventLogged) {
val source: JsonNode? = changeEventWithMetadata.eventValueAsJson()["source"]
val source: JsonNode? = changeEventWithMetadata.eventValueAsJson?.get("source")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

kotlin doesn't have ?[] unfortunately

…entexception-argument-content-is-null-after-adding-primary-key
@rodireich
Copy link
Contributor Author

@evantahler I'd like to add a troubleshooting doc for mssql, similarly to what we have in other connectors (e.g).
Is there anything I need to do other than adding the doc?

@evantahler
Copy link
Contributor

@rodireich - you've got to add the new doc to the sidebar nav as well. I can help if you get stuck!

@rodireich rodireich changed the title test MSSQL and debezium improvements Aug 28, 2024
@rodireich rodireich marked this pull request as ready for review August 28, 2024 19:53
@rodireich rodireich requested review from a team as code owners August 28, 2024 19:53
…entexception-argument-content-is-null-after-adding-primary-key
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

My only concern here is the potential for log spam, LGTM otherwise.

event
.value()
?.let { Jsons.deserialize(it) }
.also { it ?: LOGGER.warn { "Event value is null $event" } }
Copy link
Contributor

Choose a reason for hiding this comment

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

This could spam the logs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we get to the situation where value or key are null then something is seriously misoncifugred, as was the case on the oncall ticket.
I'd rather know what the malformed debezium event looks like. I couldn't figure out the root cause until I was able to get to this information.

…entexception-argument-content-is-null-after-adding-primary-key
@rodireich
Copy link
Contributor Author

rodireich commented Aug 28, 2024

/publish-java-cdk dry-run=true

🕑 https://github.com/airbytehq/airbyte/actions/runs/10605240753
✅ Successfully published Java CDK version=0.44.18!

@rodireich
Copy link
Contributor Author

rodireich commented Aug 28, 2024

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/10605307052
✅ Successfully published Java CDK version=0.44.18!

@rodireich rodireich enabled auto-merge (squash) August 28, 2024 22:33
@rodireich rodireich merged commit 18a8e5f into master Aug 28, 2024
35 checks passed
@rodireich rodireich deleted the 6192-db-sources-mssql-javalangillegalargumentexception-argument-content-is-null-after-adding-primary-key branch August 28, 2024 23:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/source/mssql
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants