Skip to content
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.

Sign up for GitHub

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

Jump to bottom

🐛 Source-MySql: do not check cdc required param binlog_row_image for standard replication #8335

Merged
merged 9 commits into from
Dec 6, 2021
Merged

🐛 Source-MySql: do not check cdc required param binlog_row_image for standard replication #8335

merged 9 commits into from
Dec 6, 2021

Conversation

mkhokh-33
Copy link
Contributor

@mkhokh-33 mkhokh-33 commented Nov 30, 2021 •
edited
Loading

What

We currently check param binlog_row_image for standard replication fro MySql connector.
This param in db should be only required for cdc replication not for standard

How

Made changes in MySqlSource method getCheckOperations in order do not check cdc required params for standard replication

Recommended reading order

  1. MySqlSource.java
  2. x.java
  3. y.json
  4. md.json

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

  • Community member? Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • docs/SUMMARY.md
    • docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
    • docs/integrations/README.md
    • airbyte-integrations/builds.md
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the connector is published, connector added to connector index as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

  • Grant edit access to maintainers (instructions)
  • Secrets in the connector's spec are annotated with airbyte_secret
  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • Code reviews completed
  • Documentation updated
    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • Credentials added to Github CI. Instructions.
  • /test connector=connectors/<name> command is passing.
  • New Connector version released on Dockerhub by running the /publish command described here
  • After the new connector version is published, connector version bumped in the seed directory as described here
  • Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -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
  • Documentation which references the generator is updated as needed.

@github-actions github-actions bot added the area/connectors Connector related issues label Nov 30, 2021
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Nov 30, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1520812854
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1520812854
No Python unittests run

@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Nov 30, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql-strict-encrypt

🕑 connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1520814297
✅ connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1520814297
No Python unittests run

@jrhizor jrhizor temporarily deployed to more-secrets November 30, 2021 12:22 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets November 30, 2021 12:22 Inactive
@mkhokh-33
Copy link
Contributor Author

@DoNotPanicUA , @andriikorotkov please pay attention only on MySqlSource.java, other changes are related to code formatting.

@mkhokh-33 mkhokh-33 marked this pull request as ready for review November 30, 2021 12:54
@mkhokh-33 mkhokh-33 linked an issue Nov 30, 2021 that may be closed by this pull request
Error connecting to MariaDB (binlog_row_image could not be queried) #7404
Closed
DoNotPanicUA
DoNotPanicUA suggested changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please exclude the changes which are not related to your change.
If there is a formatting change outside the MySql source, it should be fixed by another PR.

@andriikorotkov
Copy link
Contributor

@DoNotPanicUA, Maria asked this question in the internal chat. Sasha gave permission to add these changes in this pull request.

@mkhokh-33
Copy link
Contributor Author

@DoNotPanicUA, Maria asked this question in the internal chat. Sasha gave permission to add these changes in this pull request.

done :)

@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets November 30, 2021 13:22 Inactive
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Nov 30, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1521085644
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1521085644
No Python unittests run

@jrhizor jrhizor temporarily deployed to more-secrets November 30, 2021 13:27 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets November 30, 2021 13:44 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets November 30, 2021 14:48 Inactive
DoNotPanicUA
DoNotPanicUA approved these changes Nov 30, 2021
View reviewed changes
Copy link
Contributor

@DoNotPanicUA DoNotPanicUA left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGFM.
as a potential improvement - all three checks are very similar. we could create a method with input params and decrease code dupe a bit.

@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 1, 2021 11:31 Inactive
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 1, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1525654102
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1525654102
No Python unittests run

@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 1, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql-strict-encrypt

🕑 connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1525656193
✅ connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1525656193
No Python unittests run

@jrhizor jrhizor temporarily deployed to more-secrets December 1, 2021 12:04 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 1, 2021 12:04 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 1, 2021 12:18 Inactive
tuliren
tuliren approved these changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for DRYing the code!

tuliren
tuliren reviewed Dec 2, 2021
View reviewed changes
Copy link
Contributor

@tuliren tuliren left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it mean that the latest MySQL source connector will always fail? It does not look like the case though.

Also is this issue not caught by any unit test? I just checked that the config for MySQL source integration test is on standard replication, but does not have the binlog_row_image. Why does the integration test not fail before this PR?

@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 2, 2021 •
edited
Loading

Does it mean that the latest MySQL source connector will always fail? It does not look like the case though.

Also is this issue not caught by any unit test? I just checked that the config for MySQL source integration test is on standard replication, but does not have the binlog_row_image. Why does the integration test not fail before this PR?

In integration tests we use for testcontainer docker image version "mysql:8.0" and what I see from mysql doc 8.0 https://dev.mysql.com/doc/refman/8.0/en/replication-options-binary-log.html#sysvar_binlog_row_image that binlog_image_row is already present and by default the value is "FULL"
So I assume that test didn't fail before because there was no way to catch situation when binlog_image_row is absent or has another value then "FULL"

It can be absent for older db versions, as I can see from release notes it was introduced for mysql from 5.6 - https://dev.mysql.com/doc/refman/5.6/en/mysql-nutshell.html

On the issue users paid attention that the problem appeared for mariaDB version 5.5.60 and the changes related to binlog_image_row for MariaDB seems to be introduced latter
https://mariadb.com/kb/en/replication-and-binary-log-system-variables/#binlog_row_image for version MariaDB 10.1.6 .
version 5.5.60 is not supporting this feature yet

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 3, 2021
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 3, 2021 14:24 Inactive
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 3, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1535652835
❌ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1535652835

@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 3, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql-strict-encrypt

🕑 connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1535653645
❌ connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1535653645

@jrhizor jrhizor temporarily deployed to more-secrets December 3, 2021 14:38 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 3, 2021 14:38 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 3, 2021 17:20 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 3, 2021 17:53 Inactive
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 6, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1544272020
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1544272020

@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 6, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql-strict-encrypt

🕑 connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1544273130
✅ connectors/source-mysql-strict-encrypt https://github.com/airbytehq/airbyte/actions/runs/1544273130

@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 6, 2021 10:59 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 6, 2021 11:00 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 6, 2021 11:00 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 6, 2021 12:08 Inactive
@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 6, 2021 15:01 Inactive
@mkhokh-33
Copy link
Contributor Author

mkhokh-33 commented Dec 6, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/source-mysql

🕑 connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1545262305
✅ connectors/source-mysql https://github.com/airbytehq/airbyte/actions/runs/1545262305
No Python unittests run

@mkhokh-33 mkhokh-33 temporarily deployed to more-secrets December 6, 2021 15:19 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 6, 2021 15:19 Inactive
@mkhokh-33 mkhokh-33 merged commit 0eaaa1f into master Dec 6, 2021
@mkhokh-33 mkhokh-33 deleted the mkhokh/7404-mariabd-check-cdc-params-mysql-source branch December 6, 2021 16:33
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@mkhokh-33 @schlattk
🐛 Source-MySql: do not check cdc required param binlog_row_image for …
d7e2101 
…standard replication (airbytehq#8335)

* Source-MySql: do not check cdc required param binlog_row_image for standard replication

* Source-MySql: fix formatting

* Revert "Source-MySql: fix formatting"

This reverts commit 5a94474.

* Source-MySql: made a code improvement

* Source-MySql: bump versions

* Source-MySql: fix version in source_specs.yaml

* Source-MySql: bump versions

* Source-MySql:update source_definitions.yaml with new version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuliren tuliren tuliren approved these changes

@DoNotPanicUA DoNotPanicUA DoNotPanicUA approved these changes

@andriikorotkov andriikorotkov Awaiting requested review from andriikorotkov

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error connecting to MariaDB (binlog_row_image could not be queried)
5 participants
@mkhokh-33 @andriikorotkov @tuliren @DoNotPanicUA @jrhizor