Discover worker starts to use API to write schema result #21875

xiaohansong · 2023-01-25T19:37:29Z

What

Discover worker will stop using config repository but to use API to write schema result to DB

How

Workers to utilize the new API to write discover schema result into DB

🚨 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

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
If new credentials are required for use in CI, add them to GSM. 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
If new credentials are required for use in CI, add them to GSM. Instructions.
/test connector=connectors/<name> command is passing
New Connector version released on Dockerhub and connector version bumped by running the /publish command 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

Tests

Unit

Put your unit tests output here.

Integration

Put your integration tests output here.

Acceptance

Put your acceptance tests output here.

* migrated SAT to strictness level * fixed expected records * revert file from another source * changed extension to txt * changed extension to txt

…due to a config issue (#21144) * [19998] Destination-Bigquery: Added an explicit error message in sync fails due to a config issue

…ions on both destinations to keep them in sync (#21509)" (#21567) This reverts commit 1d202d1.

* disable tox * rename steps * revert changes on experimental workflow * do not install tox

…21589) This reverts commit 61f88f3.

Because there is a lot of CVEs in those releases. Co-authored-by: Topher Lubaway <asimplechris@gmail.com>

* add docs * add schema link * update based on feedback

* improve serviceTypeDropdownOption selector * add test ids to PathPopout component(s) * add unique id's to table dropdowns * extend submitButtonClick to support optional click options * update dropdown(pathPopout) matchers * add test-id to Overlay component * remove redundant function brackets * revert changes onSubmit button click * fix dropDown overlay issue * move all duplicated intercepters to beforeEach * add test id's to Connections, Sources and Destinations tables * add table helper functions * update source page actions * intercepter fixes * update createTestConnection function with optional replication settings * remove extra Connection name check * replace "cypress-postgres" with "pg-promise" npm package * update cypress config * Revert "update createTestConnection function with optional replication settings" This reverts commit 8e47c78. * Revert "remove extra Connection name check" This reverts commit dfb19c7. * replace openSourceDestinationFromGrid with specific selector * replace openSourceDestinationFromGrid with specific selector * turn on test * add test-id's * fix selectors * update test * update test snapshots * fix lost data-testid after resolve merge conflicts * remove extra check * move clickOnCellInTable helper to common.ts file * remove empty line and comments * fix dropdownType * replace partial string check with exact * extract interceptors and waiters to separate file * fix selector for predefined PK * fix selector * add comment regarding dropdown

…nded

xiaohansong · 2023-01-31T23:12:53Z

@pmossman For this round of review, the change besides PR comments is that I had to add a timeout limit on AirbyteApiClient in worker because sometimes requests got mysteriously lost, and a timeout could force an exception and got retried eventually.

pmossman

@xiaohansong I left a couple non-blocking comments, nice work!

pmossman · 2023-02-02T00:17:03Z

airbyte-commons-worker/src/main/java/io/airbyte/workers/config/ApiClientBeanFactory.java

@@ -53,6 +54,8 @@ public ApiClient apiClient(
        .setPort(parsePort(airbyteApiHost))
        .setBasePath("/api")
        .setHttpClientBuilder(HttpClient.newBuilder().version(Version.HTTP_1_1))
+        .setConnectTimeout(Duration.ofSeconds(30))


nit: can we define these as 'CONNECT_TIMEOUT_DURATIONandREAD_TIMEOUT_DURATION` constants?

I didn't see this comment - I made the fix in the next PR

pmossman · 2023-02-02T00:20:35Z

airbyte-integrations/bases/standard-source-test/build.gradle

@@ -13,6 +13,7 @@ import org.jsoup.Jsoup;

 dependencies {
    implementation project(':airbyte-db:db-lib')
+    implementation project(':airbyte-api')


Could this be a testImplementation rather than implementation? Is it only a dependency because it is used in the test?

Yes exactly... removed

* master: Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164)

* master: (86 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...

* master: (24 commits) Discover worker starts to use API to write schema result (#21875) 🪟 🎉 Connector Builder Landing Page (#22122) Fix pnpm cache path (#22418) Add additional shorter setup guides (#22318) Source Amazon Ads: fix reports stream records primary keys (#21677) Connector acceptance test: Fix discovered catalog caching for different configs (#22301) 🪟🐛 Make modal scrollable (#21973) only compute diff if the schema discovery actually succeeded (#22377) Source Klaviyo: fix schema (#22071) 🪟 🔧 Switch to `pnpm` for package managing (#22053) Source Sentry: turn on default availability strategy (#22303) Source freshdesk: deduplicate table names (#22164) Update connector-acceptance-tests-reference.md (#22370) Update the default security groups for the EC2 runner (#22347) Trace refresh schema operations (#22326) Remove manual docker upgrades from workflows (#22344) Update CODEOWNERS for connector acceptance tests to connector ops (#22341) 🐛 source: airtable - handle singleSelect types (#22311) Source tiktok: chunk advertiser IDs (#22309) 🪟 🧪 E2E Tests for auto-detect schema changes (#20682) ...

xiaohansong and others added 30 commits January 17, 2023 15:18

api changes for writing discover catalog

8fb6b7a

Merge remote-tracking branch 'origin/master' into xiaohan/discover

56606b5

api changes

bfdaef3

format

a7cf331

worker change 1

e11c17b

change return type of the API to return catalogId

21ec7f1

worker to call api

37080ff

Merge remote-tracking branch 'origin/master' into xiaohan/disworker

8f5ec25

Merge remote-tracking branch 'origin/master' into xiaohan/disworker

3b8e233

typo

8700b1e

🎉 Source GoogleSheets - migrated SAT to strictness level (#21399)

26a7b10

* migrated SAT to strictness level * fixed expected records * revert file from another source * changed extension to txt * changed extension to txt

🐛Destination-Bigquery: Added an explicit error message if sync fails …

a85aef7

…due to a config issue (#21144) * [19998] Destination-Bigquery: Added an explicit error message in sync fails due to a config issue

ci-connector-ops: split workflows(#21474)

ed2780c

CI: nightly build alpha sources and destinations (#21562)

d4556f1

Revert "Change main class in strict-encrypt destination and bump vers…

d54c842

…ions on both destinations to keep them in sync (#21509)" (#21567) This reverts commit 1d202d1.

Fixes webhook updating logic (#21519)

fbf6501

ci_credentials: disable tooling test run by tox (#21580)

35c83ff

* disable tox * rename steps * revert changes on experimental workflow * do not install tox

Revert "CI: nightly build alpha sources and destinations (#21562)" (#…

f0aebd8

…21589) This reverts commit 61f88f3.

Security update of default docker images (#21407)

a2b6932

Because there is a lot of CVEs in those releases. Co-authored-by: Topher Lubaway <asimplechris@gmail.com>

📝 add docs for how to add normalization (#21563)

6c46b65

* add docs * add schema link * update based on feedback

🪟 🎨 [Free connectors] Update modal copy (#21600)

3c3b935

move start/end time options out of optional block (#21541)

37ebd3a

lingering fix

05988b8

reflecting api changes

66ffbfd

test fix

3456a19

Merge remote-tracking branch 'origin/master' into xiaohan/disworker

73bc4cc

Merge remote-tracking branch 'origin/master' into xiaohan/disworker

6d577c8

worker to call api to do discover work

cbeefb9

recovered deleted html

544ebd3

Merge branch 'master' into xiaohan/disworker

d998cc2

xiaohansong temporarily deployed to more-secrets January 27, 2023 19:44 — with GitHub Actions Inactive

Merge branch 'master' into xiaohan/disworker

c6aec13

xiaohansong temporarily deployed to more-secrets January 30, 2023 18:03 — with GitHub Actions Inactive

Merge remote-tracking branch 'origin/master' into xiaohan/disworker

2851158

xiaohansong temporarily deployed to more-secrets January 30, 2023 22:36 — with GitHub Actions Inactive

api client needs to have a timeout in case request does not get respo…

036db6c

…nded

xiaohansong temporarily deployed to more-secrets January 31, 2023 22:32 — with GitHub Actions Inactive

xiaohansong requested a review from pmossman January 31, 2023 23:11

xiaohansong requested a review from mfsiega-airbyte January 31, 2023 23:21

pmossman approved these changes Feb 2, 2023

View reviewed changes

Merge branch 'master' into xiaohan/disworker

947baeb

xiaohansong temporarily deployed to more-secrets February 6, 2023 16:55 — with GitHub Actions Inactive

xiaohansong merged commit 36698ce into master Feb 6, 2023

xiaohansong deleted the xiaohan/disworker branch February 6, 2023 17:35

github-actions bot mentioned this pull request Feb 7, 2023

Bump helm chart version reference to 0.43.28 #22489

Closed

This was referenced Feb 8, 2023

Bump Airbyte version from 0.40.32 to 0.40.33 #22578

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22689

Closed

Bump Airbyte version from 0.40.32 to 0.40.33 #22839

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discover worker starts to use API to write schema result #21875

Discover worker starts to use API to write schema result #21875

xiaohansong commented Jan 25, 2023 •

edited

Loading

xiaohansong commented Jan 31, 2023

pmossman left a comment

pmossman Feb 2, 2023

xiaohansong Feb 6, 2023

pmossman Feb 2, 2023

xiaohansong Feb 2, 2023

Discover worker starts to use API to write schema result #21875

Discover worker starts to use API to write schema result #21875

Conversation

xiaohansong commented Jan 25, 2023 • edited Loading

What

How

Recommended reading order

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

Tests

xiaohansong commented Jan 31, 2023

pmossman left a comment

Choose a reason for hiding this comment

pmossman Feb 2, 2023

Choose a reason for hiding this comment

xiaohansong Feb 6, 2023

Choose a reason for hiding this comment

pmossman Feb 2, 2023

Choose a reason for hiding this comment

xiaohansong Feb 2, 2023

Choose a reason for hiding this comment

xiaohansong commented Jan 25, 2023 •

edited

Loading