-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
MySQL Source : add CDC heartbeat support #23984
Conversation
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
/test connector=connectors/source-mysql
Build PassedTest summary info:
|
Affected Connector ReportNOTE
|
Connector | Version | Changelog | Publish |
---|---|---|---|
source-alloydb |
2.0.3 |
✅ | ✅ |
source-alloydb-strict-encrypt |
2.0.3 |
🔵 (ignored) |
🔵 (ignored) |
source-mssql |
1.0.4 |
✅ | ✅ |
source-mssql-strict-encrypt |
1.0.4 |
🔵 (ignored) |
🔵 (ignored) |
source-mysql |
2.0.6 |
✅ | ✅ |
source-mysql-strict-encrypt |
2.0.6 |
🔵 (ignored) |
🔵 (ignored) |
source-postgres |
2.0.4 |
✅ | ✅ |
source-postgres-strict-encrypt |
2.0.4 |
🔵 (ignored) |
🔵 (ignored) |
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Destinations (0)
Connector | Version | Changelog | Publish |
---|
- See "Actionable Items" below for how to resolve warnings and errors.
✅ Other Modules (0)
Actionable Items
(click to expand)
Category | Status | Actionable Item |
---|---|---|
Version | ❌ mismatch |
The version of the connector is different from its normal variant. Please bump the version of the connector. |
⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
|
Changelog | ⚠ doc not found |
The connector does not seem to have a documentation file. This can be normal (e.g. basic connector like source-jdbc is not published or documented). Please double-check to make sure that it is not a bug. |
❌ changelog missing |
There is no chnagelog for the current version of the connector. If you are the author of the current version, please add a changelog. | |
Publish | ⚠ not in seed |
The connector is not in the seed file (e.g. source_definitions.yaml ), so its publication status cannot be checked. This can be normal (e.g. some connectors are cloud-specific, and only listed in the cloud seed file). Please double-check to make sure that it is not a bug. |
❌ diff seed version |
The connector exists in the seed file, but the latest version is not listed there. This usually means that the latest version is not published. Please use the /publish command to publish the latest version. |
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.
@VitaliiMaltsev I have pushed a couple of commits to simplify the implementation. Basically I introduced generics which makes it simpler. Can you carry out some manual testing to make sure this works as expected. Once done please share the results and I will approve
/test connector=connectors/source-mysql
Build FailedTest summary info:
|
/test connector=connectors/source-postgres
Build PassedTest summary info:
|
/test connector=connectors/source-mssql
Build PassedTest summary info:
|
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.
Looks good to me, just a few minor comments
...rations/bases/debezium/src/main/java/io/airbyte/integrations/debezium/CdcTargetPosition.java
Outdated
Show resolved
Hide resolved
* @return true if lsn is equal or greater than target lsn | ||
*/ | ||
default boolean reachedTargetPosition(final Long lsn) { | ||
default boolean reachedTargetPosition(final T positionFromHeartbeat) { |
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 we just remove the default?
It just throw an exception as it's an interface and all the classes needs to implement it.
Otherwise we can make default the method extractPositionFromHeartbeatOffset
.
|
||
assertEquals(lsn, 358824993496L); | ||
assertEquals(-1, debeziumRecordIterator.getHeartbeatPosition(null)); |
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.
we should keep this test, but instead of making it equal to -1
to null
.
if (heartbeatEvent == null) { | ||
return null; | ||
} |
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 is not redundant is an early exit to reduce unneeded computation and object creation.
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 is not redundant is an early exit to reduce unneeded computation and object creation.
@sergio-ropero please take a look here
this check for null is definitely not needed since it can never occur
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 and no, so it will be never null if you include it inside an if statement that previously checks it's not null.
I think we can remove it, but we need to ensure that we always include that if statement.
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 and no, so it will be never null if you include it inside an if statement that previously checks it's not null.
I think we can remove it, but we need to ensure that we always include that if statement.
i don't understand what do you mean to "ensure that we always include that if statement" if we will never get null here
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.
What I mean is that they are 2 different functions, and it will never be null because in our current code we are including the function getHeartbeatPosition
inside the if statement that uses isHeartbeatEvent
.
That means that the function getHeartbeatPosition
can be called (in other scenarios) with a null object, except if again we include it inside a block code with the same if statement.
@subodh1810 СDC syncs started to fail permanently with the error Caused by: java.io.StreamCorruptedException: unexpected EOF in middle of data block Probably the root cause of these failing is not your changes but @sergio-ropero PR UPD these failures are not related to your or mine changes. MySQL CDC syncs are failing in master branch with the same error |
I experience the same issue for big tables from MySQL to Snowflake. Can you please let me know if there is any issue with the latest version of Airbyte (v.0.42.0) and the latest MySQL Source (v2.0.3)? From the conversation above and @VitaliiMaltsev comments here and in issue #24191 it is not clear. |
@dimoschi i would like to ask you to do 2 more experiments and provide your results. It will be very helpful to detect the root cause
|
Thanks for the immediate response. I wasn't able to build source-mysql. First tests failed and then after executing
|
I don't know if that helps, but using |
Thank you @VitaliiMaltsev for bringing that up in the first place. I haven't done much and unfortunately, I couldn't spend more time resolving build issues with the |
Lets track and continue the communication for the EOF error on issue #24191 |
@VitaliiMaltsev I think this PR is safe to merge and we dont expect to fix the EOF error as part of this PR. We will continue to work on it as part of the issue. So lets get this PR merged. |
/publish connector=connectors/source-mysql
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/publish connector=connectors/source-mysql-strict-encrypt
if you have connectors that successfully published but failed definition generation, follow step 4 here |
/test connector=connectors/source-mysql
Build PassedTest summary info:
|
/publish connector=connectors/source-mysql run-tests=false
if you have connectors that successfully published but failed definition generation, follow step 4 here |
* MySQL Source : add CDC heartbeat support * removed logs * fixed DebeziumRecordIteratorTest * Automated Change * use generics to simplify implementation * use Duration * more refactoring * removed redundant null check * Automated Change * bump version * auto-bump connector version --------- Co-authored-by: VitaliiMaltsev <VitaliiMaltsev@users.noreply.github.com> Co-authored-by: subodh <subodh1810@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
* MySQL Source : add CDC heartbeat support * removed logs * fixed DebeziumRecordIteratorTest * Automated Change * use generics to simplify implementation * use Duration * more refactoring * removed redundant null check * Automated Change * bump version * auto-bump connector version --------- Co-authored-by: VitaliiMaltsev <VitaliiMaltsev@users.noreply.github.com> Co-authored-by: subodh <subodh1810@gmail.com> Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
What
Issue 23257
🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereUpdating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereConnector Generator
-scaffold
in their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates
then checking in your changes