Skip to content

MINOR: improve error message for Serde type miss match#6801

Merged
mjsax merged 2 commits into
apache:trunkfrom
mjsax:minor-improve-serde-error-message
May 28, 2019
Merged

MINOR: improve error message for Serde type miss match#6801
mjsax merged 2 commits into
apache:trunkfrom
mjsax:minor-improve-serde-error-message

Conversation

@mjsax

@mjsax mjsax commented May 23, 2019

Copy link
Copy Markdown
Member

Atm, if the type does not match, the error is miss leading and not helpful:

A serializer (value: org.apache.kafka.streams.state.internals.ValueAndTimestampSerializer) is not compatible to the actual value type (value type: org.apache.kafka.streams.state.ValueAndTimestamp). Change the default Serdes in StreamConfig or provide correct Serdes via method parameters.

This PR makes sure that the actual value type information is in the message to make the ValueAndTimestamp wrapping transparent to the user and to make the error message useful.

This should be cherry-picked to 2.3 branch.

@mjsax mjsax added the streams label May 23, 2019
@mjsax

mjsax commented May 23, 2019

Copy link
Copy Markdown
Member Author

@ableegoldman ableegoldman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

One nit, otherwise LGTM!

} else {
serializerClass = valueSerializer().getClass();
valueClass = value == null ? "unknown because value is null" : value.getClass().getName();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: I felt we can remove the value in (value: %s) below?

assertThat(
e.getMessage(),
equalTo(
"A serializer (value: org.apache.kafka.common.serialization.StringSerializer) " +

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Could we reuse the error message or the entire template?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We could reuse for this and the second "value" test, but not for the "key" test. Don't see big value is sharing though.

@mjsax

mjsax commented May 24, 2019

Copy link
Copy Markdown
Member Author

Updated this. Will merge if Jenkins is green again.

@mjsax

mjsax commented May 24, 2019

Copy link
Copy Markdown
Member Author

Retest this please

@mjsax

mjsax commented May 24, 2019

Copy link
Copy Markdown
Member Author

Java8 passed. Java11 failed with know flaky test.

Retest this please.

@mjsax

mjsax commented May 28, 2019

Copy link
Copy Markdown
Member Author

Java8 passed. Java11 failed (test results not available any longer).

Retest this please.

@mjsax
mjsax merged commit 88619b7 into apache:trunk May 28, 2019
@mjsax
mjsax deleted the minor-improve-serde-error-message branch May 28, 2019 18:39
mjsax added a commit that referenced this pull request May 28, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
@mjsax

mjsax commented May 28, 2019

Copy link
Copy Markdown
Member Author

Merged to trunk and cherry-picked to 2.3 branch.

haidangdam pushed a commit to haidangdam/kafka that referenced this pull request May 29, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
pengxiaolong pushed a commit to pengxiaolong/kafka that referenced this pull request Jun 14, 2019
Reviewers: A. Sophie Blee-Goldman <sophie@confluent.io>, Guozhang Wang <guozhang@confluent.io>, Boyang Chen <boyang@confluent.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants