-
Notifications
You must be signed in to change notification settings - Fork 58
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
Enable use of unqualified table names in source connector #223
Conversation
823e62d
to
6dc7f4a
Compare
6dc7f4a
to
3b682b2
Compare
3b682b2
to
c640f4a
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.
LGTM +1, just left some minor comments on the config name. Let me know if it make sense. Thanks @C0urante !
src/main/java/io/aiven/connect/jdbc/source/JdbcSourceConnectorConfig.java
Outdated
Show resolved
Hide resolved
config.getString(JdbcSourceConnectorConfig.INCREMENTING_COLUMN_NAME_CONFIG); | ||
if (!qualifyTableNames && (incrementingColumn == null || incrementingColumn.isEmpty())) { | ||
// Otherwise, we may infer the wrong incrementing key for the table | ||
// TODO: This restraint is not necessary in all cases, but additional logic will be required to |
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 this be tracked as an issue?
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 on the fence. IMO filing an issue implies that there is immediate action that can/should be taken; this is more of a note about a potential improvement we can investigate in the future if necessary.
...tionTest/java/io/aiven/kafka/connect/jdbc/postgres/UnqualifiedTableNamesIntegrationTest.java
Show resolved
Hide resolved
@jeqo thanks for the review! I've updated the config name. Still not sure about ticket vs. TODO; if you have strong feelings one way or the other, can do whatever makes you happy. |
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.
Make sense, just wanted to make sure we are not missing anything 👍🏽
Thanks @C0urante !
Implements #219
Depends on #222
A new
table.names.qualify
property is introduced, with a default oftrue
(which preserves existing behavior). When set tofalse
:Existing unit tests are adapted/expanded to cover fine-grained logic for this feature, and a new
UnqualifiedTableNamesIntegrationTest
integration test suite is added to test a few scenarios including different query modes and preservation of offsets across restarts.