-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[source-postgres] : Provide option to advance LSN #34781
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Before Merging a Connector Pull RequestWow! What a great pull request you have here! 🎉 To merge this PR, ensure the following has been done/considered for each connector added or updated:
If the checklist is complete, but the CI check is failing,
|
Coverage report for source-postgres
|
@@ -68,6 +68,11 @@ private static Properties commonProperties(final JdbcDatabase database) { | |||
: HEARTBEAT_INTERVAL; | |||
props.setProperty("heartbeat.interval.ms", Long.toString(heartbeatInterval.toMillis())); | |||
|
|||
if (sourceConfig.get("replication_method").has("heartbeat_action_query") | |||
&& !sourceConfig.get("replication_method").get("heartbeat_action_query").asText().isEmpty()) { | |||
props.setProperty("heartbeat.action.query", sourceConfig.get("replication_method").get("heartbeat_action_query").asText()); |
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.
This seems straightforward configuring an existing debezium property.
I'm just concerned we're opening a door to all sorts of direct sql manipulation.
Not sure how but would be safer to solve it in some templated way so users or some bad actor cannot inject something.
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.
If most of all customers will end up configuring
INSERT INTO airbyte_heartbeat (text) VALUES ('heartbeat')
Can we maybe change field to input something like [schema].[table] and have our code build a query?
could be reasons not 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.
Discussed offline - I'd like to not make things harder than they are by forcing users to create a specific table. Thinking here is they could just reuse one they already have instead of creating one. WDYT?
@@ -68,6 +68,11 @@ private static Properties commonProperties(final JdbcDatabase database) { | |||
: HEARTBEAT_INTERVAL; | |||
props.setProperty("heartbeat.interval.ms", Long.toString(heartbeatInterval.toMillis())); | |||
|
|||
if (sourceConfig.get("replication_method").has("heartbeat_action_query") |
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.
Can we add heartbeat_action_query
to a test - even by just configuring it on a test to make sure nothing is broken.
@@ -289,6 +289,13 @@ | |||
], | |||
"default": "After loading Data in the destination", | |||
"order": 7 | |||
}, | |||
"heartbeat_action_query": { |
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.
Checking: because this field is optional, existing connection lacking it would just keep working
i.e non-breaking change
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.
Yes - this property is only set if it's populated, otherwise I've verified that the behavior is the same. The default value is empty
Closes #32946
An option is exposed to set the Debezium property in the setup config : heartbeat.action.query
Set up instructions for users are :
Tested locally. Some notes :