Skip to content

[DB-sources] : Improve heartbeat logic #40516

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
Merged
merged 23 commits into from
Jul 1, 2024
Merged

[DB-sources] : Improve heartbeat logic #40516

merged 23 commits into from
Jul 1, 2024
+79 −74

Conversation

@akashkulk
Copy link
Contributor

@akashkulk akashkulk commented Jun 25, 2024 •
edited
Loading

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/8255

  • Remove heartbeat logic in prod - it is still require for in testing
  • Add analytics messages for shutdown reasons
  • Remove subsequent wait times
@vercel
Copy link

vercel bot commented Jun 25, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
airbyte-docs ⬜️ Ignored (Inspect) Visit Preview Jul 1, 2024 6:58pm
@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/postgres labels Jun 25, 2024
@akashkulk akashkulk marked this pull request as ready for review June 26, 2024 00:20
@akashkulk akashkulk requested review from a team as code owners June 26, 2024 00:20
@theyueli
Copy link
Contributor

It seems the PR description is not very accurate. It looks like instead of removing heartbeat logic, we added a switch that allows us to ignore HB errors. The heartbeat still happens.

I have a general question: if we do not exercise them in production, why do we still want them in the test? Why not simply remove those codes, as we no longer need the HB?
@akashkulk
Copy link
Contributor Author

It seems the PR description is not very accurate. It looks like instead of removing heartbeat logic, we added a switch that allows us to ignore HB errors. The heartbeat still happens.

I have a general question: if we do not exercise them in production, why do we still want them in the test? Why not simply remove those codes, as we no longer need the HB?

Unfortunately, we require heartbeat in the tests. The goal here is to rely on the platform to shut down the sync if no records are emitted.

However, in there are some unit tests where we want to force this heartbeat shutdown behavior - and we cannot lean on the platform. Having the debezium level heartbeat shutdown in this case helps simulates the "sync not progressing" case
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jun 29, 2024
@akashkulk akashkulk requested a review from theyueli June 29, 2024 21:05
theyueli
theyueli approved these changes Jul 1, 2024
View reviewed changes
Copy link
Contributor

@theyueli theyueli left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you!
@akashkulk
Copy link
Contributor Author

akashkulk commented Jul 1, 2024 •
edited by github-actions bot
Loading

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/9748985412
❌ Publish Java CDK version=0.40.6 failed!
@akashkulk
Copy link
Contributor Author

akashkulk commented Jul 1, 2024 •
edited by github-actions bot
Loading

/publish-java-cdk

🕑 https://github.com/airbytehq/airbyte/actions/runs/9749550912
✅ Successfully published Java CDK version=0.40.7!
@edgao
Copy link
Contributor

edgao commented Jul 1, 2024

@akashkulk did you mean to publish 0.40.6?
@akashkulk
Copy link
Contributor Author

akashkulk commented Jul 1, 2024 •
edited
Loading

@akashkulk did you mean to publish 0.40.6?

Ah I did! but 0.40.6 failed. Turns out it was due to a compilation error (but I assumed it was due to version mismatch).

If you have a PR in flight, can you publish 0.40.8? I think this PR should be in v soon so you can merge the changes in. SOrry for the confusion
@akashkulk akashkulk merged commit 3a3e058 into master Jul 1, 2024
@akashkulk akashkulk deleted the akash/dbz-close branch July 1, 2024 19:51
@edgao
Copy link
Contributor

edgao commented Jul 1, 2024

cool, will do. I cancelled my 0.40.6 publish before it did anything, so we should be in a reasonable state
xiaohansong pushed a commit that referenced this pull request Jul 9, 2024
@akashkulk @xiaohansong
[DB-sources] : Improve heartbeat logic (#40516)
009db17
@gingeard gingeard mentioned this pull request Jul 16, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/source/mongodb-v2 connectors/source/mssql connectors/source/mysql connectors/source/postgres

5 participants

@akashkulk @theyueli @edgao @octavia-squidington-iii