Skip to content

KAFKA-8523 Enabling InsertField transform to be used with tombstone events#6914

Merged
rhauch merged 2 commits intoapache:trunkfrom
gunnarmorling:KAFKA-8523
Oct 3, 2019
Merged

KAFKA-8523 Enabling InsertField transform to be used with tombstone events#6914
rhauch merged 2 commits intoapache:trunkfrom
gunnarmorling:KAFKA-8523

Conversation

@gunnarmorling
Copy link
Copy Markdown
Contributor

Fixes https://issues.apache.org/jira/browse/KAFKA-8523

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

Hey @rhauch, a minor fix to the InsertFieldSMT -- Feedback welcome!

@ftardif
Copy link
Copy Markdown

ftardif commented Sep 17, 2019

So, does an insert config on a tombstone <k,null> will still produce a <k, null> (hence ignored), or will it produce a <k, v> where v contains only the inserted fields. If it does not propagate the null value, I am concerned it will skew the elasticsearch connector config behavior.on.null.values
that can be used to explicitly delete index entries on null.

image

Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

Thanks, @gunnarmorling! This looks great, but I have two minor suggestions.

@ftardif
Copy link
Copy Markdown

ftardif commented Sep 18, 2019

before merging this PR, can we discuss the impact of applying transformation on null values? I strongly believe we should rather propagate the null values untouched even when transformations are configured.

see: https://issues.apache.org/jira/browse/KAFKA-8523?focusedCommentId=16931777&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16931777

Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

@ftardif, please see the discussion on https://issues.apache.org/jira/browse/KAFKA-8523. I think @gunnarmorling, you, and I all agree that this PR should be changed to pass through and not modify tombstone records.

This PR has not yet been approved, but just to be safe I've marked this PR as Request changes.

@ftardif
Copy link
Copy Markdown

ftardif commented Sep 25, 2019

@gunnarmorling are you planning to provide an adjusted PR soon in order to start testing this functionality?

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

gunnarmorling commented Sep 25, 2019 via email

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

gunnarmorling commented Sep 27, 2019

@rhauch, @ftardif, I've updated the PR and force-pushed as per the discussion on the JIRA issue; tombstones are returned unmodified now by the InsertField transformation. I have also addressed the other review remarks regarding imports and method naming.

@ftardif
Copy link
Copy Markdown

ftardif commented Oct 1, 2019

thanks @gunnarmorling . What are the next steps to get that PR merged?

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

Good question; I suppose approval by @rhauch or any other committer should do the trick.

Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks, @gunnarmorling!

I plan to merge shortly.

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

gunnarmorling commented Oct 2, 2019 via email

@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Oct 2, 2019

@gunnarmorling yes, I hope to get this into the AK 2.4.0 release. For information about release plans, checkout the dev list email thread with subject [DISCUSS] Apache Kafka 2.4.0 release (or the specific release you're interested in), which points to the AK 2.4.0 release plan.

Copy link
Copy Markdown
Contributor

@rhauch rhauch left a comment

Choose a reason for hiding this comment

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

LGTM.

@rhauch rhauch merged commit e767703 into apache:trunk Oct 3, 2019
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
rhauch pushed a commit that referenced this pull request Oct 3, 2019
…vents (#6914)

* KAFKA-8523 Avoiding raw type usage

* KAFKA-8523 Gracefully handling tombstone events in InsertField SMT
@rhauch
Copy link
Copy Markdown
Contributor

rhauch commented Oct 3, 2019

Merged back as far back as the 1.0 branch. See https://issues.apache.org/jira/browse/KAFKA-8523 for details.

@gunnarmorling
Copy link
Copy Markdown
Contributor Author

gunnarmorling commented Oct 3, 2019 via email

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