-
Notifications
You must be signed in to change notification settings - Fork 0
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
Drop invalid messages if needed #91
Conversation
It looks like @patsak hasn't signed our Contributor License Agreement, yet.
You can read and sign our full Contributor License Agreement here. Once you've signed reply with Appreciation of efforts, clabot |
Can one of the admins verify this patch? |
@patsak Are there cases where we expect to fail to convert the data? I'm not sure whether a setting for this makes sense -- if there's a bug in the conversion code, we should just fix that. |
@ewencp Producer has written message without key for example - https://github.com/confluentinc/kafka-connect-elasticsearch/blob/master/src/main/java/io/confluent/connect/elasticsearch/DataConverter.java#L56-L58 |
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.
@patsak Ok, that makes sense. Do you want to rebase and re-target to 3.4.x? That'd get the change released most quickly.
As it is, the change looks good aside from merge conflicts.
f1089c2
to
7cbbf17
Compare
7cbbf17
to
733f9e2
Compare
[clabot:check] |
@confluentinc It looks like @patsak just signed our Contributor License Agreement. 👍 Always at your service, clabot |
@ewencp I've retargeted branch with merge conflicts resolving and style fixes. |
ok to test |
Great, looks good. Thanks for the contribution @patsak! |
No description provided.