Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Dead letter table #233
base: main
Are you sure you want to change the base?
Dead letter table #233
Changes from 32 commits
03c3e36
e87d820
b062c51
9b7f6b0
c1b4de2
4e706e1
f9136d8
d3905a5
18c79ba
77aad4b
edc75a5
8ee6840
de479d3
b78a68d
ac5d6a5
50b300b
01f8cbb
d89f15a
c5c2186
831f205
451e20f
41c4372
13220e9
797b861
7bd7d5d
bff7233
25208da
205c2d7
4aeedcd
0cf18d1
05ea87f
457a2f3
1518cc7
e4977c4
4036557
0ff7972
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Example config with Dead Letter will be very useful
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.
Docs can always be improved. I'll try to take another stab at this.
This is a big feature with a somewhat clunky coinfig API due to config visibility rules in Kafka Connect, so more docs/examples certainly help.
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.
You don't need any of the actual iceberg functionality in this module?
The only thing you do need is this import :D
Which IMO you can just replace with this safely.
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 believe you're de-risking the chance of a collision with an existing header by prefixing a
t_
More out of curiosity, what does the
t_
stand for?And wondering if we can do a little better.
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.
Also, we could de-risk collisions more by just adding a single header
t_original_record
which is a Struct with this kind of structure (psuedo-code) instead of adding 3 separate headers:nit: I would also name the header something specific to iceberg-kafka-connect IDK something along the lines of
kafka.connect.iceberg.error.transform.original.record
or something (obviously this is too long but you get the idea).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.
You are correct in derisking collisions. I chose
t
for tabular.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.
🤦 should have figured that one out ....
Large diffs are not rendered by default.
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.
Shouldn't live in this module, should live in the dead-letter module
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.
WriteExceptionHandler is only used by the connector, not by both the connector and the error transform --it doesn't need to be shared.
Since it all gets packaged into a single jar anyways.... I could move it, or leave it separate. I'm ok with it how it is unless this really bugs you for some reason.
Note: we have other open comments about modules so let's chat offline about all that.
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.
The
WriteExceptionHandler
interface should live in the connector, that's fine.I wanted the implementation/s of that interface (currently just
DefaultWriteExceptionHandler
) to live outside of the connector because it's expected to be pluggable and because I'm not convinced we can get the implementation correct the first time (it's extremely hard to figure out what is transient and what is not transient). Once we're confident we've got it correct, we can move it in and make it the default if we want but that's a later step IMO.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.
If I move it, I also have to move all the custom exceptions into iceberg-kafka-connect-deadletter as well, otherwise I can't reference them from the exception handler.
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.
That's concerning because that means other users defining their own
WriteExceptionHandler
implementations can't reference those exceptions either?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.
Let me think about this. If
iceberg-kafka-connect
runtime wasprovided
, they should be able to see this no?