-
Notifications
You must be signed in to change notification settings - Fork 13.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
KAFKA-6685: Added Exception to distinguish message Key from Value during deserializing. #4765
KAFKA-6685: Added Exception to distinguish message Key from Value during deserializing. #4765
Conversation
…ing deserializing to Kafka connect format.
…ing deserialization to Kafka connect format.
retest this please |
Hi @rhauch, |
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.
@jadireddi thanks for creating this PR and jumping in to add the better exception logic. I do have a suggestion for making this more readable and concise.
log.error("Failed to convert message Value to Kafka Connect format", e); | ||
} | ||
throw new ConnectException("Exiting WorkerSinkTask due to unconverted message to Kafka Connect format exception", e); | ||
} |
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.
Perhaps the following would be a bit more straightforward without the boolean flag while providing more meaningful content in the error message?
final SchemaAndValue keyAndSchema;
final SchemaAndValue valueAndSchema;
try {
keyAndSchema = keyConverter.toConnectData(msg.topic(), msg.key());
} catch (Throwable e) {
String str = String.format("Error converting message key in topic '%s' partition %d at offset %d and timestamp %d",
msg.topic(), msg.partition(), msg.offset(), msg.timestamp());
throw new ConnectException(str, e);
}
try {
valueAndSchema = valueConverter.toConnectData(msg.topic(), msg.value());
} catch (Throwable e) {
String str = String.format("Error converting message value in topic '%s' partition %d at offset %d and timestamp %d",
msg.topic(), msg.partition(), msg.offset(), msg.timestamp());
throw new ConnectException(str, e);
}
Also, note that we don't want to log the exception here since exceptions will ultimately be caught and logged in WorkerTask.doRun()
. Logging it here is unnecessary.
And actually, since there's a fair amount of duplicate logic, a better way of reducing that logic is to extract it into a method:
SchemaAndValue keyAndSchema = toConnectData(keyConverter, "key", msg, msg.key());
SchemaAndValue valueAndSchema = toConnectData(valueConverter, "value", msg, msg.value());
with the method:
private SchemaAndValue toConnectData(Converter converter, String converterName, ConsumerRecord<byte[], byte[]> msg, byte[] data) {
try {
return converter.toConnectData(msg.topic(), data);
} catch (Throwable e) {
String str = String.format("Error converting message %s in topic '%s' partition %d at offset %d and timestamp %d",
converterName, msg.topic(), msg.partition(), msg.offset(), msg.timestamp());
throw new ConnectException(str, e);
}
}
This is even more concise and readable. WDYT?
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.
Thank you @rhauch for your time to review and provide valuable suggestions. Code looks really concise and readable. Making changes.
Hi @rhauch , Incorporated review comments. Kindly review them. |
…ing deserializing to Kafka connect format.
…ing deserialization to Kafka connect format.
…lue' of https://github.com/jadireddi/kafka into KAFKA-6685---log-message-should-distinguish-key-from-value
…essage-should-distinguish-key-from-value
@rhauch, Can you please review changes and let me know if anything needed. |
retest this please |
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.
@jadireddi one more suggestion, but otherwise looks good.
final SchemaAndValue valueAndSchema; | ||
|
||
keyAndSchema = toConnectData(keyConverter, "key", msg, msg.key()); | ||
valueAndSchema = toConnectData(valueConverter, "value", msg, msg.value()); |
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.
How about just declaring and initializing each variable in one statement?
SchemaAndValue keyAndSchema = toConnectData(keyConverter, "key", msg, msg.key());
SchemaAndValue valueAndSchema = toConnectData(valueConverter, "value", msg, msg.value());
Also, in this case I'm not sure final
adds much if any value.
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.
@rhauch, Fixed review comments. It's true final
doesn't add much value in this case..Removed it. Thank you for the review comments..
…-6685---log-message-should-distinguish-key-from-value
…-6685---log-message-should-distinguish-key-from-value
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.
LGTM. @ewencp, would you mind reviewing and merging if all looks good?
Thanks!
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.
LGTM, thanks for the contribution to debuggability!
…ing deserializing. https://issues.apache.org/jira/browse/KAFKA-6685 Added Exception message in `WorkerSinkTask.convertMessages` to distinguish message Key from Value during deserialization to Kafka connect format. *More detailed description of your change, if necessary. The PR title and PR message become the squashed commit message, so use a separate comment to ping reviewers.* *Summary of testing strategy (including rationale) for the feature or bug fix. Unit and/or integration tests are expected for any behaviour change and system tests should be considered for larger changes.* Author: Jagadesh Adireddi <adireddijagadesh@gmail.com> Reviewers: Randall Hauch <rhauch@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#4765 from jadireddi/KAFKA-6685---log-message-should-distinguish-key-from-value
https://issues.apache.org/jira/browse/KAFKA-6685
Added Exception message in
WorkerSinkTask.convertMessages
to distinguish message Key from Value during deserialization to Kafka connect format.More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)