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

Fixes #127. Added delete support for sink. #282

Open
wants to merge 3 commits into
base: 3.3.x
from

Conversation

Projects
None yet
@andybryant
Contributor

andybryant commented Sep 13, 2017

A record with null value is considered a delete, much like Kafka Connect.
Deletes are disabled by default. Set config delete.enabled to true to enable. Also requires pk.mode = record_key otherwise it can't find the record to delete.

@ConfluentJenkins

This comment has been minimized.

Contributor

ConfluentJenkins commented Sep 13, 2017

Can one of the admins verify this patch?

@Analect

This comment has been minimized.

Analect commented Oct 10, 2017

@ewencp ... would it be possible to have someone review this delete capability?

@niketanand123

This comment has been minimized.

niketanand123 commented Jan 16, 2018

Hi Andy,
When do you see this Delete fix going to release? I am not sure why it is the least priority

@kdrakon

This comment has been minimized.

kdrakon commented Jan 17, 2018

bumping this...
this feature is desired by a client using the connector

@niketanand123

This comment has been minimized.

niketanand123 commented Jan 17, 2018

Hi Kdrakon,
Could you provide the time frame for pushing this to release version?

@kdrakon

This comment has been minimized.

kdrakon commented Jan 17, 2018

@niketanand123 I don't work for Confluent; I'm in the same boat as you - waiting for this to be approved and released.

@niketanand123

This comment has been minimized.

niketanand123 commented Jan 17, 2018

@andybryant , Please let me know when can we see your fix going to release version.

We really need the basic operation to work in JDBC sink connector.

@andybryant

This comment has been minimized.

Contributor

andybryant commented Jan 17, 2018

@niketanand123 I'm waiting for a review of this. Happy to port to confluent 4.0.0 branch or refactor if needed, but haven't had any feedback yet.

@andybryant

This comment has been minimized.

Contributor

andybryant commented Jan 17, 2018

For what it's worth I've been using this code in anger for several months backed by some property based integration tests and haven't seen any issues.

@ewencp ewencp requested a review from confluentinc/connect Jan 18, 2018

@gunnarmorling

This comment has been minimized.

gunnarmorling commented Jan 19, 2018

Just came cross this PR and wanted to express that it'd be great to have this feature (haven't looked at the proposed implementation).

It's a common request we see when using the JDBC sink connector with the Debezium CDC connectors, where people would like to get delete events applied, too (We provide an SMT for extracting just the "after" state from Debezium's CDC events, allowing to emit the format expected by this sink connector. Currently that SMT must be configured to drop any tombstone events in order to work with the JDBC sink).

So we'd be excited to see this implemented!

@niketanand123

This comment has been minimized.

niketanand123 commented Jan 19, 2018

@gunnarmorling Could you provide the time frame for pushing this to release version?

@gunnarmorling

This comment has been minimized.

gunnarmorling commented Jan 19, 2018

@niketanand123, that's a decision left to the Confluent engineers; I just wanted to express that it's a feature welcomed by the community very much.

@VMarisevs

This comment has been minimized.

VMarisevs commented Feb 8, 2018

bump...
Hope it will be merged anytime soon!!

@RanKey1496

This comment has been minimized.

RanKey1496 commented Mar 1, 2018

Waiting for this feature, when this Delete fix going to release?

@minh5

This comment has been minimized.

minh5 commented Apr 13, 2018

Any word/update on this feature?

@jakubbujny

This comment has been minimized.

jakubbujny commented Sep 17, 2018

Any plans for merge?

@ian-axelrod

This comment has been minimized.

ian-axelrod commented Sep 18, 2018

Christ, address this already.

@gunnarmorling

This comment has been minimized.

gunnarmorling commented Sep 18, 2018

@ian-axelrod

This comment has been minimized.

ian-axelrod commented Sep 18, 2018

@gunnarmorling Can you explain how that would be an improvement over the solution presented in this PR? A teammate of mine is actually working on a fork of this plugin this very moment, so if he can take advantage of data provided by Debezium in a way that would simplify this PR's logic, I would love for him to do so.

@gunnarmorling

This comment has been minimized.

gunnarmorling commented Sep 18, 2018

Can you explain how that would be an improvement over the solution presented in this PR?

@ian-axelrod, for sure a fix for this in the JDBC sink connector itself would be more than welcomed. As this feature is something that we've been asked about many times, we figured that for the time being, optionally rewriting DELETEs into an update of a given marker column provides a way to unblock users that wish to replicate deletions (and it also can be used with other sink connectors that can't handle deletes). In addition, working with a rewrite marker column allows you to keep "deleted" records in a sink for some time before deleting them there, too, which might or might no be a useful feature.

I'd suggest to continue this discussion in our Google group if needed, so keep the thread here focused on the proposed change for the JDBC sink connector.

@RabihHallage

This comment has been minimized.

RabihHallage commented Sep 26, 2018

are there any plans to merge this PR soon ?

@ian-axelrod

This comment has been minimized.

ian-axelrod commented Sep 26, 2018

@RabihHallage given that confluent has not paid any attention to this PR, my guess is it will be merged either (1) never or (2) ... never. My team has forked the repo and added delete support to the latest version of the connector. Feel free to use the fork. https://github.com/dailymuse/kafka-connect-jdbc

@aymehri

This comment has been minimized.

aymehri commented Sep 26, 2018

Need this functionality too... Any update?

@u-phoria

This comment has been minimized.

u-phoria commented Sep 26, 2018

I have little doubt Confluent folks are aware of this thread. It has to be one of the "hotter" ones in this project.

It is a non trivial change, so as reluctance to just merge it in is understandable.

However just plain ignoring it is poor form, and doesn't inspire confidence. I hope this changes soon.

@rhauch

This comment has been minimized.

Member

rhauch commented Sep 28, 2018

I'm hoping to get to this soon to see if this PR can be easily rebased.

@mcamenzind

This comment has been minimized.

mcamenzind commented Oct 25, 2018

@rhauch Have any update regarding this PR? Please see @ian-axelrod 's fork, which seems easier to rebase.

@rhauch

This comment has been minimized.

Member

rhauch commented Oct 25, 2018

@mcamenzind, I started looking into it, but had trouble rebasing and adjusting for the recent changes. I'll take a look at @ian-axelrod's fork -- do you have a link? Is there a PR or just commits on their fork?

@mcamenzind

This comment has been minimized.

mcamenzind commented Oct 26, 2018

@rhauch, I squashed the commits on master of [1] back including e7430a3 and rebased it, I think there was exactly one conflict with one documentation file, no issues otherwise. It compiles, I did, however, not get the occasion to test it thoroughly yet.

There 's no PR, just their fork. To cite @ian-axelrod:

@RabihHallage given that confluent has not paid any attention to this PR, my guess is it will be merged either (1) never or (2) ... never. My team has forked the repo and added delete support to the latest version of the connector. Feel free to use the fork. https://github.com/dailymuse/kafka-connect-jdbc

I just found this other PR #488 [2], which is based on [1]. You could probably abandon this one (PR #282) and just rebase and merge #488. Please note that [3] has some comment on the code.
#127 could then be also closed.

[1] https://github.com/dailymuse/kafka-connect-jdbc/commits/master
[2] #488
[3] dailymuse@8ccbe5e#r30931000

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment