Implement Tombstone Message Handling for JDBC Sink Connector #302

Joel-hanson · 2024-04-24T10:54:20Z

Description:

This pull request addresses issue #165, focusing on enhancing the functionality of the JDBC sink connector to handle deletes on tombstone messages effectively. By implementing this feature, users can now delete rows corresponding to tombstone messages, which is particularly useful for scenarios involving Change Data Capture (CDC) from another database.

Changes:

Added support for handling tombstone messages in the JDBC sink connector.
Implemented the ability to delete rows based on tombstone messages.
Introduced a new parameter, delete.enabled, to control delete behavior.
Aligned functionality with the documented approach for processing tombstones, similar to Confluent JDBC driver behavior.

Related Issue(s):

Fixes Handle deletes on tombstone messages #165

Signed-off-by: Joel Hanson joelhanson025@gmail.com

Joel-hanson · 2024-05-02T05:27:59Z

@ivanyu, could you please take a look at this pull request when you get a chance? Your feedback would be really valuable. Thank you!

matuagarwal · 2024-05-02T06:04:15Z

@jeqo @ivanyu could you please review the PR

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

davidradl · 2024-05-16T10:43:16Z

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

-        if (records.isEmpty()) {
-            log.debug("Records is empty");
+        if (records.isEmpty() && tombstoneRecords.isEmpty()) {
+            log.debug("Records are empty.");


nit
I am not sure about this debug.

the debug text should include Records and tombstone records are empty.

Do we want to have a debug is the records are empty as before and there are tombstones .

Do we want to have a debug is the records are empty as before and there are tombstones.

It seems redundant to include if statements solely for debugging empty records, as the prepare statement debug (mentioned previously) will already cover this aspect by not debugging the statement if no records are there.

davidradl · 2024-05-16T11:12:40Z

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

+            } else {
+                records.add(record);
+            }
+            if (records.size() >= config.batchSize || tombstoneRecords.size() >= config.batchSize) {
                log.debug("Flushing buffered records after exceeding configured batch size {}.",


nit
might be worth adding in the records.size() and tombstoneRecords.size() so we know which one caused the flush

Thanks @davidradl for the review, I have addressed most of the comments

aindriu-aiven · 2024-05-17T13:23:33Z

Hey @Joel-hanson, thanks a million for your contribution I have this on my list for review on Monday, sorry for the delay I am just catching up on a few projects at the moment!

aindriu-aiven · 2024-05-20T10:17:43Z

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

+                records.add(record);
+            }
+            if (records.size() >= config.batchSize || tombstoneRecords.size() >= config.batchSize) {
+                log.debug("Flushing buffered records {} and tombstone records {} after exceeding the configured batch size of {}.",


Hey,
Just started going through the PR but there are a couple of checkstyle errors on line 102 & Line 124 for lines that exceed 120 characters.

C0urante

Thanks @Joel-hanson for the PR! I've left a few thoughts but overall it looks pretty good.

I'm unable to run the integration tests on my Apple Silicon Macbook, though. I've tried several things (including looking through Testcontainers issues and using an alternative Docker runtime) with no success.

Would it be possible to use Postgres either instead of or in addition to Oracle for the new integration tests? As Apple Silicon becomes more and more prevalent for devs the cost of having tests that can't be run locally increases.

build.gradle.kts

src/main/java/io/aiven/connect/jdbc/dialect/DatabaseDialect.java

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

src/main/java/io/aiven/connect/jdbc/sink/JdbcSinkConfig.java

Joel-hanson · 2024-05-29T04:28:50Z

@aindriu-aiven @C0urante I have addressed most of the comments. Could you please do a re-review?

C0urante

Thanks @Joel-hanson! This is looking really good. I've given all the changes to the main codebase and integration tests a pass. Still haven't looked at the unit tests yet; will try to do that in the next pass once these comments are addressed.

Also, please be sure to build the project locally before pushing more commits. You can do this with ./gradlew clean build; right now there are still a handful of Checkstyle errors that are causing the build to fail.

src/main/java/io/aiven/connect/jdbc/dialect/DatabaseDialect.java

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java

src/main/java/io/aiven/connect/jdbc/JdbcSinkConnector.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyInsertIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyDeleteIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/postgres/VerifyDeleteIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyDeleteIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/AbstractOracleIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyDeleteIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyInsertIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/postgres/VerifyInsertIT.java

src/integrationTest/java/io/aiven/kafka/connect/jdbc/oracle/VerifyDeleteIT.java

C0urante

LGTM, thanks @Joel-hanson!

One last request--can you squash your commits into a single commit? No need to mention "address review comments" or anything like that, we just need a description that'll be helpful in the git history for the master branch after the merge.

- Add support for handling tombstone messages in the JDBC sink connector. - Implement ability to delete rows based on tombstone messages. - Introduce new parameter `delete.enabled` to control delete behavior. - Align functionality with documented approach for processing tombstones, similar to Confluent JDBC driver behavior. - Add new integration test with Oracle database. - Implement integration tests for PostgreSQL and Oracle database to test insert and delete operations. - Add new sink config validation for delete enabled config. - Updated the docs to include the new config for sink connector. Signed-off-by: Joel Hanson <joelhanson025@gmail.com> Signed-off-by: Joel Hanson <joel.hanson2@ibm.com>

Joel-hanson · 2024-06-11T05:44:57Z

LGTM, thanks @Joel-hanson!

One last request--can you squash your commits into a single commit? No need to mention "address review comments" or anything like that, we just need a description that'll be helpful in the git history for the master branch after the merge.

@C0urante Thanks, I have squashed the commit and updated the documentation file to include the delete.enabled configuration.

Joel-hanson · 2024-06-13T06:08:41Z

@C0urante Thank you for merging PR. I was wondering if you could provide an estimate of when the next release will be published that includes these changes?

If there's anything I can do to assist with the release process, please let me know.

Thank you again for your support and for maintaining this project.

C0urante · 2024-06-17T14:13:20Z

@Joel-hanson I've reached out to some people internally, will let you know when I hear back.

Joel-hanson requested review from a team as code owners April 24, 2024 10:54

Joel-hanson mentioned this pull request Apr 25, 2024

Tombstone messages are being ignored (and not handled as per documentation) #287

Closed

Joel-hanson force-pushed the support-delete branch from 085a497 to d7e51af Compare May 2, 2024 05:23

davidradl reviewed May 16, 2024

View reviewed changes

src/main/java/io/aiven/connect/jdbc/sink/BufferedRecords.java Show resolved Hide resolved

davidradl reviewed May 16, 2024

View reviewed changes

Joel-hanson force-pushed the support-delete branch from d7e51af to 5e2a3c8 Compare May 17, 2024 07:52

Joel-hanson requested a review from davidradl May 17, 2024 07:53

aindriu-aiven reviewed May 20, 2024

View reviewed changes

C0urante reviewed May 20, 2024

View reviewed changes

Joel-hanson force-pushed the support-delete branch 2 times, most recently from 2a27255 to 3aab7d2 Compare May 28, 2024 06:44

Joel-hanson requested review from C0urante and aindriu-aiven May 28, 2024 06:46

C0urante reviewed May 29, 2024

View reviewed changes

Joel-hanson force-pushed the support-delete branch from 3aab7d2 to 5ef719d Compare June 4, 2024 09:13

C0urante reviewed Jun 4, 2024

View reviewed changes

Joel-hanson force-pushed the support-delete branch from 5ef719d to 43b2959 Compare June 7, 2024 09:45

Joel-hanson requested a review from C0urante June 7, 2024 11:20

C0urante previously approved these changes Jun 10, 2024

View reviewed changes

Joel-hanson dismissed C0urante’s stale review via 4a10120 June 11, 2024 05:42

Joel-hanson force-pushed the support-delete branch from 43b2959 to 4a10120 Compare June 11, 2024 05:42

Joel-hanson requested a review from C0urante June 11, 2024 05:43

C0urante approved these changes Jun 11, 2024

View reviewed changes

C0urante merged commit bb19998 into Aiven-Open:master Jun 11, 2024
3 checks passed

Joel-hanson deleted the support-delete branch June 13, 2024 06:06

AnatolyPopov mentioned this pull request Jul 15, 2024

Release version 6.10.0 #341

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Tombstone Message Handling for JDBC Sink Connector #302

Implement Tombstone Message Handling for JDBC Sink Connector #302

Joel-hanson commented Apr 24, 2024

Joel-hanson commented May 2, 2024

matuagarwal commented May 2, 2024

davidradl May 16, 2024

Joel-hanson May 17, 2024 •

edited

Loading

davidradl May 16, 2024

Joel-hanson May 17, 2024

aindriu-aiven commented May 17, 2024

aindriu-aiven May 20, 2024

C0urante left a comment

Joel-hanson commented May 29, 2024

C0urante left a comment •

edited

Loading

C0urante left a comment

Joel-hanson commented Jun 11, 2024

Joel-hanson commented Jun 13, 2024

C0urante commented Jun 17, 2024

Implement Tombstone Message Handling for JDBC Sink Connector #302

Implement Tombstone Message Handling for JDBC Sink Connector #302

Conversation

Joel-hanson commented Apr 24, 2024

Description:

Changes:

Joel-hanson commented May 2, 2024

matuagarwal commented May 2, 2024

davidradl May 16, 2024

Choose a reason for hiding this comment

Joel-hanson May 17, 2024 • edited Loading

Choose a reason for hiding this comment

davidradl May 16, 2024

Choose a reason for hiding this comment

Joel-hanson May 17, 2024

Choose a reason for hiding this comment

aindriu-aiven commented May 17, 2024

aindriu-aiven May 20, 2024

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

Joel-hanson commented May 29, 2024

C0urante left a comment • edited Loading

Choose a reason for hiding this comment

C0urante left a comment

Choose a reason for hiding this comment

Joel-hanson commented Jun 11, 2024

Joel-hanson commented Jun 13, 2024

C0urante commented Jun 17, 2024

Joel-hanson May 17, 2024 •

edited

Loading

C0urante left a comment •

edited

Loading