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

fix mysql-cdc: handle tinyint(1) and boolean correctly + fix target file comparison #3890

Merged
merged 8 commits into from
Jun 7, 2021
Merged

fix mysql-cdc: handle tinyint(1) and boolean correctly + fix target file comparison #3890

merged 8 commits into from
Jun 7, 2021

Conversation

subodh1810
Copy link
Contributor

Issue : #3804

The PR

  1. Improves the target file logic and fixes how binlog file name from record was being compared to the target filename.
  2. Changes the way TinyInt and Boolean were handled in CDC to the same way as its being handled in full refresh

Recommended reading order

  1. x.java
  2. y.python

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste 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.
  • /test command documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Documentation updated
    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

@subodh1810 subodh1810 self-assigned this Jun 4, 2021
@auto-assign auto-assign bot requested review from cgardens and sherifnada June 4, 2021 12:53
@subodh1810
Copy link
Contributor Author

subodh1810 commented Jun 4, 2021 •
edited by github-actions bot
Loading

/test connector=source-mysql

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

sherifnada
sherifnada approved these changes Jun 4, 2021
View reviewed changes
.../source-mysql/src/main/java/io/airbyte/integrations/source/mysql/DebeziumRecordIterator.java Outdated
Jsons.deserialize(event.value()).get("source").get("snapshot").asText()
.toUpperCase());
LOGGER.info(
"Signalling close cause record's binlog file : " + file + " , position : " + position
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Signalling close cause record's binlog file : " + file + " , position : " + position
"Signalling close because record's binlog file : " + file + " , position : " + position

...tors/source-mysql/src/test/java/io/airbyte/integrations/source/mysql/CdcMySqlSourceTest.java Outdated
})
.collect(Collectors.toSet());

JsonNode data1 = Jsons.jsonNode(ImmutableMap.of(
Copy link
Contributor

Choose a reason for hiding this comment

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

just a small comment for maintainability: can we return these values from (or input them to) the setupforComparison method so they are not defined in separate places like this?

@subodh1810
Copy link
Contributor Author

subodh1810 commented Jun 7, 2021 •
edited by github-actions bot
Loading

/test connector=source-mysql

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

@subodh1810 subodh1810 mentioned this pull request Jun 7, 2021
Merged
1 task
@subodh1810
Copy link
Contributor Author

subodh1810 commented Jun 7, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql

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

@subodh1810
Copy link
Contributor Author

subodh1810 commented Jun 7, 2021 •
edited by github-actions bot
Loading

/publish connector=connectors/source-mysql

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

@subodh1810 subodh1810 merged commit 4e084a1 into master Jun 7, 2021
@subodh1810 subodh1810 deleted the mysql-cdc-tinyint-handling branch June 7, 2021 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sherifnada sherifnada sherifnada approved these changes

@cgardens cgardens Awaiting requested review from cgardens

Assignees

@subodh1810 subodh1810

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@subodh1810 @sherifnada