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.

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

#23949 Remove check() from read() method for sources Cockroach DB and Postgres #24000

Merged
merged 16 commits into from
Mar 15, 2023

Conversation

nguyenaiden
Copy link
Contributor

@nguyenaiden nguyenaiden commented Mar 13, 2023

What

Currently, we call check() during the read() method for sources CockroachDB and Postgres. This forces a interdependency between protocol methods.
Issue: https://app.zenhub.com/workspaces/db--dw-source-connectors-6333360e0a41155061efbcbd/issues/gh/airbytehq/airbyte/23949
Solution:

  • Remove the check method and offload that responsibility to the platform
  • Removed testReadWithoutPublication and testReadWithoutReplicationSlot in CdcPostgresSourceTest since they were related to the check() call. These are now redundant since we already have testCheckWithoutReplicationSlot and testCheckWithoutPublication

@CLAassistant
Copy link

CLAassistant commented Mar 13, 2023

CLA assistant check
All committers have signed the CLA.

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 13, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4408610490
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4408610490
🐛 https://gradle.com/s/telzimdmzzezk

Build Failed

Test summary info:

Could not find result summary

@github-actions
Copy link
Contributor

github-actions bot commented Mar 13, 2023

Affected Connector Report

NOTE ⚠️ Changes in this PR affect the following connectors. Make sure to do the following as needed:

  • Run integration tests
  • Bump connector or module version
  • Add changelog
  • Publish the new version

✅ Sources (5)

Connector Version Changelog Publish
source-alloydb 2.0.2
source-alloydb-strict-encrypt 2.0.2 🔵
(ignored)
🔵
(ignored)
source-cockroachdb 0.1.21
source-cockroachdb-strict-encrypt 0.1.21 🔵
(ignored)
🔵
(ignored)
source-postgres-strict-encrypt 2.0.3 🔵
(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.

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 13, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4408850395
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4408850395
🐛 https://gradle.com/s/6iew6l2jhqr26

Build Failed

Test summary info:

Could not find result summary

@nguyenaiden nguyenaiden force-pushed the duy/remove-check-postgres-cockroach branch from a597070 to e92d933 Compare March 13, 2023 20:11
@akashkulk
Copy link
Contributor

akashkulk commented Mar 13, 2023

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4409296926
❌ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4409296926
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
=================== 62 passed, 5 skipped in 80.74s (0:01:20) ===================

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 13, 2023

/test connector=connectors/source-cockroachdb

🕑 connectors/source-cockroachdb https://github.com/airbytehq/airbyte/actions/runs/4409547850
❌ connectors/source-cockroachdb https://github.com/airbytehq/airbyte/actions/runs/4409547850
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================= 19 passed, 6 skipped, 27 warnings in 16.50s ==================

@nguyenaiden nguyenaiden marked this pull request as ready for review March 14, 2023 20:23
@nguyenaiden nguyenaiden requested a review from a team as a code owner March 14, 2023 20:23
@prateekmukhedkar
Copy link
Contributor

Update postgres.md and cockroachdb.md in docs/integration/sources with the changelog.

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Mar 14, 2023
@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 14, 2023

/test connector=connectors/source-postgres

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

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
=================== 62 passed, 5 skipped in 82.18s (0:01:22) ===================

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 14, 2023

/test connector=connectors/source-cockroachdb

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

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================= 19 passed, 6 skipped, 27 warnings in 18.14s ==================

@@ -95,6 +95,7 @@ Your database user should now be ready for use with Airbyte.

| Version | Date | Pull Request | Subject |
|:--------|:-----------| :--- | :--- |
| 0.1.21 | 2022-03-14 | [24000](https://github.com/airbytehq/airbyte/pull/24000) | Removed check method call on read. |
Copy link
Contributor

Choose a reason for hiding this comment

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

fix year in date :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh good call! On it now.

@@ -396,6 +396,7 @@ The root causes is that the WALs needed for the incremental sync has been remove

| Version | Date | Pull Request | Subject |
|:--------|:-----------|:---------------------------------------------------------|:---------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| 2.0.3 | 2022-03-14 | [24000](https://github.com/airbytehq/airbyte/pull/24000) | Removed check method call on read. |
Copy link
Contributor

Choose a reason for hiding this comment

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

fix year, i suggest correcting all the ones showing up as 2022 since the start of this year.

connectors.md Outdated
@@ -78,6 +78,7 @@
| **Freshdesk** | <img alt="Freshdesk icon" src="https://raw.githubusercontent.com/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/icons/freshdesk.svg" height="30" height="30"/> | Source | airbyte/source-freshdesk:3.0.2 | generally_available | [link](https://docs.airbyte.com/integrations/sources/freshdesk) | [code](https://github.com/airbytehq/airbyte/tree/master/airbyte-integrations/connectors/source-freshdesk) | <small>`ec4b9503-13cb-48ab-a4ab-6ade4be46567`</small> |
| **Freshsales** | <img alt="Freshsales icon" src="https://raw.githubusercontent.com/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/icons/freshsales.svg" height="30" height="30"/> | Source | airbyte/source-freshsales:0.1.2 | alpha | [link](https://docs.airbyte.com/integrations/sources/freshsales) | [code](https://github.com/airbytehq/airbyte/tree/master/airbyte-integrations/connectors/source-freshsales) | <small>`eca08d79-7b92-4065-b7f3-79c14836ebe7`</small> |
| **Freshservice** | <img alt="Freshservice icon" src="https://raw.githubusercontent.com/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/icons/freshservice.svg" height="30" height="30"/> | Source | airbyte/source-freshservice:0.1.1 | alpha | [link](https://docs.airbyte.com/integrations/sources/freshservice) | [code](https://github.com/airbytehq/airbyte/tree/master/airbyte-integrations/connectors/source-freshservice) | <small>`9bb85338-ea95-4c93-b267-6be89125b267`</small> |
| **GCS** | <img alt="GCS icon" src="https://raw.githubusercontent.com/airbytehq/airbyte/master/airbyte-config/init/src/main/resources/icons/gcs.svg" height="30" height="30"/> | Source | airbyte/source-gcs:0.1.0 | unknown | [link](https://docs.airbyte.com/integrations/sources/gcs) | [code](https://github.com/airbytehq/airbyte/tree/master/airbyte-integrations/connectors/source-gcs) | <small>`2a8c41ae-8c23-4be0-a73f-2ab10ca1a820`</small> |
Copy link
Contributor

Choose a reason for hiding this comment

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

GCS was unaffected by this PR, yet it shows up here. I believe this happens when the feature branch is out of date of master. Once you merge this section should be removed. I'd double check before merging the PR in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Merging in master fixed it.

@@ -99,11 +99,6 @@ public AutoCloseableIterator<AirbyteMessage> read(final JsonNode config,
final ConfiguredAirbyteCatalog catalog,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can just rid of the read() method override here and below in general, since all it's doing is delegating to the parent class

Copy link
Contributor

@akashkulk akashkulk 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, just a quick comment!

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/test connector=connectors/source-cockroachdb

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

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================= 19 passed, 6 skipped, 27 warnings in 16.16s ==================

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/test connector=connectors/source-postgres

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

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:509: The previous and actual discovered catalogs are identical.
=================== 62 passed, 5 skipped in 80.04s (0:01:20) ===================

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/publish connector=connectors/source-postgres run-tests=false

🕑 Publishing the following connectors:
connectors/source-postgres
https://github.com/airbytehq/airbyte/actions/runs/4431246446


Connector Did it publish? Were definitions generated?
connectors/source-postgres

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/publish connector=connectors/source-postgres-strict-encrypt run-tests=false

🕑 Publishing the following connectors:
connectors/source-postgres-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/4431247520


Connector Did it publish? Were definitions generated?
connectors/source-postgres-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/publish connector=connectors/source-cockroachdb run-tests=false

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/publish connector=connectors/source-cockroachdb-strict-encrypt run-tests=false

🕑 Publishing the following connectors:
connectors/source-cockroachdb-strict-encrypt
https://github.com/airbytehq/airbyte/actions/runs/4431249249


Connector Did it publish? Were definitions generated?
connectors/source-cockroachdb-strict-encrypt

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@nguyenaiden
Copy link
Contributor Author

nguyenaiden commented Mar 15, 2023

/publish connector=connectors/source-cockroachdb run-tests=false

🕑 Publishing the following connectors:
connectors/source-cockroachdb
https://github.com/airbytehq/airbyte/actions/runs/4431603372


Connector Did it publish? Were definitions generated?
connectors/source-cockroachdb

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@nguyenaiden nguyenaiden enabled auto-merge (squash) March 15, 2023 23:15
@nguyenaiden nguyenaiden merged commit 2e85c94 into master Mar 15, 2023
@nguyenaiden nguyenaiden deleted the duy/remove-check-postgres-cockroach branch March 15, 2023 23:23
adriennevermorel pushed a commit to adriennevermorel/airbyte that referenced this pull request Mar 17, 2023
…ch DB and Postgres (airbytehq#24000)

* Removed extraneous read on the check() method for CockroachDB and Postgres
* Bumped dockerfile version for relevant conenctors
* Bumped versions for strick-encrypt tests
* Removed withoutPublication and withoutReplicationSlot tests for postgres cdc
* Automated Change
* Updated sources docs
* Update incorrect year in change logs doc for Cockroach and Postgres
* Removed read override method from cockroachDB
* Removed read method override for source-postgres
* auto-bump connector version

---------

Co-authored-by: nguyenaiden <nguyenaiden@users.noreply.github.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
… Postgres (#24000)

* Removed extraneous read on the check() method for CockroachDB and Postgres
* Bumped dockerfile version for relevant conenctors
* Bumped versions for strick-encrypt tests
* Removed withoutPublication and withoutReplicationSlot tests for postgres cdc
* Automated Change
* Updated sources docs
* Update incorrect year in change logs doc for Cockroach and Postgres
* Removed read override method from cockroachDB
* Removed read method override for source-postgres
* auto-bump connector version

---------

Co-authored-by: nguyenaiden <nguyenaiden@users.noreply.github.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
erohmensing pushed a commit that referenced this pull request Mar 22, 2023
… Postgres (#24000)

* Removed extraneous read on the check() method for CockroachDB and Postgres
* Bumped dockerfile version for relevant conenctors
* Bumped versions for strick-encrypt tests
* Removed withoutPublication and withoutReplicationSlot tests for postgres cdc
* Automated Change
* Updated sources docs
* Update incorrect year in change logs doc for Cockroach and Postgres
* Removed read override method from cockroachDB
* Removed read method override for source-postgres
* auto-bump connector version

---------

Co-authored-by: nguyenaiden <nguyenaiden@users.noreply.github.com>
Co-authored-by: Octavia Squidington III <octavia-squidington-iii@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants