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
DBZ-3621 adding JdbcDatabaseHistory, JdbcOffsetBackingStore #2543
Conversation
@ismailsimsek Thank you for the PR. Wouldn't it make more sense to add the stores into |
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.
Some quick first comments. Thx!
...erver/debezium-server-core/src/main/java/io/debezium/server/storage/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
...erver/debezium-server-core/src/main/java/io/debezium/server/storage/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
...erver/debezium-server-core/src/main/java/io/debezium/server/storage/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
...er/debezium-server-core/src/main/java/io/debezium/server/storage/JdbcOffsetBackingStore.java
Outdated
Show resolved
Hide resolved
...er/debezium-server-core/src/main/java/io/debezium/server/storage/JdbcOffsetBackingStore.java
Outdated
Show resolved
Hide resolved
c9cf74e
to
b2521eb
Compare
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.
Thank you @jpechane @gunnarmorling for the reviews, its ready for another review
try { | ||
log.debug("Saving data to state table..."); | ||
conn.setAutoCommit(false); | ||
try (PreparedStatement sqlDelete = conn.prepareStatement(OFFSET_STORAGE_TABLE_DELETE);) { |
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.
looking at FileOffsetBackingStore
i believe save method doing override?
should we put some effort to support saving offsets by multiple, different debezium processes? (supporting multiple databases)
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'm not quite following; the offset store would only ever be invoked in the context of single connectors.
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 mean if there is more than one replication running, (2 jvm independent processes replicating mysql.database_1 and oracle.database_2) could we use same jdbc table for both replications as a FileOffsetBackingStore? would that be good feature to add?
one option could be reading table name from application.properties so user could provide different table name per replication
assertThat(tables.size() == 1); | ||
assertThat(tables.forTable(tableId).equals(table)); | ||
history.record(source, position2, historyRecord2.databaseName(), historyRecord2.schemaName(), historyRecord2.ddl(), tableChanges2); | ||
history.record(source, position2, historyRecord2.databaseName(), historyRecord2.schemaName(), historyRecord2.ddl(), tableChanges2); |
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.
not sure why recording it once not returning table2
? had to call it twice to make test pass
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
b2521eb
to
bfb9e24
Compare
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.
@ismailsimsek, a few more comments, we're getting there. Could you also look into adding a documentation page, probably something under https://debezium.io/documentation/reference/1.7/.
@jpechane, if you have specific preferences on how this should be represented in user docs, please speak up :)
Thanks a lot, @ismailsimsek!
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/storage/JdbcOffsetBackingStore.java
Outdated
Show resolved
Hide resolved
try { | ||
log.debug("Saving data to state table..."); | ||
conn.setAutoCommit(false); | ||
try (PreparedStatement sqlDelete = conn.prepareStatement(OFFSET_STORAGE_TABLE_DELETE);) { |
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'm not quite following; the offset store would only ever be invoked in the context of single connectors.
@gunnarmorling IMHO This needs either a separate page or a section in Debezium Engine. The page/section should describe all implementations that are now on the plate - file-based, Kafka and JDBC. |
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.
@ismailsimsek, took another look, please see my comments inline. I've also pushed a commit with some stylistic changes I did along the way.
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-core/src/main/java/io/debezium/relational/history/JdbcDatabaseHistory.java
Outdated
Show resolved
Hide resolved
debezium-embedded/src/main/java/io/debezium/embedded/storage/JdbcOffsetBackingStore.java
Outdated
Show resolved
Hide resolved
debezium-embedded/src/main/java/io/debezium/embedded/storage/JdbcOffsetBackingStore.java
Outdated
Show resolved
Hide resolved
debezium-core/src/test/java/io/debezium/relational/history/JdbcDatabaseHistoryTest.java
Outdated
Show resolved
Hide resolved
9428d21
to
01a6687
Compare
9e450e7
to
9530c5a
Compare
9530c5a
to
922cdf5
Compare
922cdf5
to
04683f8
Compare
Hey, have any plans to merge this pr? |
Hi, @gunnarmorling I am also curious about the state of this PR. This feature would really help us. Thanks a lot!! |
Thanks for the push, @tomsej and @wingerx. We can certainly try to revive this for 1.9. It has been a while since I last looked into this PR, so I'll need to refresh my memory first. There were some problems with unwanted dependencies established by this, not sure whether they have been sorted out by now, or not. |
@ismailsimsek @tomsej @wingerx Hi, I'd like to get this one for 2.0. I've merged the PR that provides modularized offset and history storages. Would any of you be willing to retrofit to it? Thanks |
21f3a5c
to
eefb62a
Compare
…ations DBZ-3621 adding JdbcDatabaseHistory, JdbcOffsetBackingStore implementations
691d892
to
797d208
Compare
@ismailsimsek Please check #4005 to see the plugins needed in the pom file to get the assembly built. Try it locally and then please fine tune deps to make sure that Debezium modules and deps not needed by the storage module are not packed in. |
@ismailsimsek The S3 PR has been merged. Do you think you'd be able to find few spare cycles so we can get this one out too? |
|
@ismailsimsek , any ideas on how I can take over this work, only way I know is to create another PR from my fork. Guessing I wont have permission to commit to this branch. Thanks |
Thanks, got the invite to your repo. |
797d208
to
0c5e4f0
Compare
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
…imsek/debezium into dbz_server_jdbc_storage
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
1 similar comment
Hi @ismailsimsek, thanks for your contribution. Please prefix the commit message(s) with the DBZ-xxx JIRA issue key. |
@ismailsimsek @subkanthi I cleaned the PR a bit and removed the merge commits, please let's continue at #4212 |
https://issues.redhat.com/browse/DBZ-3621