-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Fail without enqueueing iff the airbyte message type is unrecognized #40254
Fail without enqueueing iff the airbyte message type is unrecognized #40254
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple nitpicks, LGTM once those are resolved
FYI how to publish the cdk:
- https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/java/airbyte-cdk/README.md#bumping-the-cdk-version
- and also add an entry to the changelog at the bottom
...-cdk/core/src/test/kotlin/io/airbyte/cdk/integrations/base/AirbyteTraceMessageUtilityTest.kt
Outdated
Show resolved
Hide resolved
...ore/src/test/kotlin/io/airbyte/cdk/integrations/destination/async/AsyncStreamConsumerTest.kt
Show resolved
Hide resolved
airbyte-cdk/java/airbyte-cdk/dependencies/src/main/kotlin/io/airbyte/commons/json/Jsons.kt
Show resolved
Hide resolved
...ore/src/test/kotlin/io/airbyte/cdk/integrations/destination/async/AsyncStreamConsumerTest.kt
Show resolved
Hide resolved
if (match != null) { | ||
val unrecognized = match.groups[1]?.value | ||
logger.warn { "Unrecognized message type: $unrecognized" } | ||
return null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather throw a typed exception than return null here. This way, we also avoid all the !! everywhere we know this to be valid
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
37f5987
to
f3bb580
Compare
@stephane-airbyte I think your comments have been addressed. |
What
Addresses this Zenhub issue: when the airbyte message type is unrecognized, log an error instead of throwing the exception.
How
NOTE: This means that we will never call enqueue with the byte size passed into accept. However, @stephane-airbyte noticed that we do nothing with this when the type is unrecognized anyway (which may be a mistake if something's relying on that matching the calls to accept).
cc @edgao
User Impact
User should notice very little. In the rare event we add a new message type, it will not be a breaking change.
Can this PR be safely reverted and rolled back?
Yes, as long as we are not in the middle of the process of releasing a new message type. :)