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: add serialization exceptions to processing logger #6084

Merged
merged 5 commits into from
Aug 25, 2020

Conversation

vcrfxia
Copy link
Contributor

@vcrfxia vcrfxia commented Aug 24, 2020

Description

Fixes #5364

Two changes in this PR (corresponding to the first two commits):

  • add serialization exceptions to the processing log: currently deserialization exceptions are logged but serialization exceptions aren't
  • throw a more informative error message in the event that serialization exceptions are caused by UDFs with return types with non-optional schemas, since the error message from Connect (Struct schemas do not match.) isn't particularly actionable by users.

Testing done

Added unit tests. Manual testing underway.

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

Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM! What's the current behavior on error for push queries?

@vcrfxia
Copy link
Contributor Author

vcrfxia commented Aug 24, 2020

What's the current behavior on error for push queries?

Not sure I understand the question. Push queries use the same serializers and deserializers as persistent queriers so the logging behavior is the same. The name of the logger contains the query ID, which for the push queries is a random large number (in contrast to the sensible query IDs of persistent queries).

If you're asking about the push query behavior for the particular linked issue that this PR fixes, push queries don't generate serialization errors in the examples in the issue since the example push queries don't write to any Kafka topics, so the serializer (where the error is generated) is unused.


message.deserializationError.topic (STRING)

: The Kafka topic of the record for which deserialization failed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: The Kafka topic of the record for which deserialization failed.
: The {{ site.ak }} topic of the record for which deserialization failed.


message.serializationError.topic (STRING)

: The Kafka topic to which the ksqlDB row that failed to serialize
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: The Kafka topic to which the ksqlDB row that failed to serialize
: The {{ site.ak }} topic to which the ksqlDB row that failed to serialize

Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

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

LGTM, with a few suggestions.

@vcrfxia vcrfxia merged commit 8ab98a5 into confluentinc:master Aug 25, 2020
@vcrfxia vcrfxia deleted the udf-nonoptional-schema branch August 25, 2020 14:36
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.

UDFs that returns non-optional data types silently fail in persistent queries
3 participants