Add routing logic for check and discover workflow #21822

xiaohansong · 2023-01-24T20:52:42Z

What

Check and discover workflow will now use a routing logic to decide which task queue to enqueue the job.

How

Expanding routeService to support check/discover.
Inject routeService to server app.

🚨 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.

No - changes should be invisible to user.

Tested with https://github.com/airbytehq/airbyte-cloud/pull/4185

* 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 <[email protected]>

* 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

pmossman · 2023-01-27T21:59:40Z

...ns-temporal/src/main/java/io/airbyte/commons/temporal/scheduling/DefaultTaskQueueMapper.java

+import java.util.Map;
+
+@Singleton
+public class DefaultTaskQueueMapper implements TaskQueueMapper {


I wonder if it would be easier to maintain a different approach here - my rough idea is that instead of maintaining a separate map for every job type going forward and adding a new RouterService method for each job type, perhaps the RouterService and TaskQueueMapper should be more opinionated and force a standard task queue format based on the geography and TemporalJobType name.

So, instead of having a separate map and method for every type of job, we could instead have a single method getTaskQueue(TemporalJobType, Geography) which returns the appropriate task queue. The DefaultTaskQueueMapper could simply return the TemporalJobType name without considering the Geography at all, and the CloudTaskQueueMapper could have a map from Geography to taskQueueSuffix that it adds to the TemporalJobType name to get the appropriate task queue.

Does that make sense? I haven't reviewed this full PR but I want to leave this comment up front to see what you think - I think it will make it easier to use/test/maintain the RouterService and TaskQueueMapper classes, and it forces some level of convention around how we name task queues for different jobs and geographies. Let me know what you think!

Sure, thanks for the suggestion. That's done!

pmossman

Looks good to me! I left some minor nits and suggestions

...ver/src/main/java/io/airbyte/commons/server/scheduler/DefaultSynchronousSchedulerClient.java

pmossman · 2023-02-03T00:15:00Z

...-config/config-persistence/src/main/java/io/airbyte/config/persistence/ConfigRepository.java

@@ -1544,6 +1544,14 @@ public Geography getGeographyForConnection(final UUID connectionId) throws IOExc
        .fetchOneInto(Geography.class);
  }

+  public Geography getGeographyForWorkspace(final UUID workspaceId) throws IOException {


I don't see this used in this PR, is it called in a different PR or in Cloud? Also probably worth adding a quick unit test for this method

Yes - going to be, because at the stage of discover/check we will only have workspace Id available to query which region we should use. Will add a test.

airbyte-workers/src/main/java/io/airbyte/workers/ApplicationInitializer.java

pmossman · 2023-02-03T00:23:38Z

airbyte-workers/src/test/java/io/airbyte/commons/temporal/scheduling/RouterServiceTest.java

-    Mockito.when(mTaskQueueMapper.getTaskQueue(Geography.EU)).thenReturn(EU_TASK_QUEUE);
+    Mockito.when(mTaskQueueMapper.getTaskQueue(Geography.AUTO, TemporalJobType.SYNC)).thenReturn(US_TASK_QUEUE);
+    Mockito.when(mTaskQueueMapper.getTaskQueue(Geography.US, TemporalJobType.SYNC)).thenReturn(US_TASK_QUEUE);
+    Mockito.when(mTaskQueueMapper.getTaskQueue(Geography.EU, TemporalJobType.SYNC)).thenReturn(EU_TASK_QUEUE);
  }

  @Test


You could turn this into a @ParameterizedTest where it takes in an @EnumSource for TemporalJobType and verifies that it works correctly for everything, instead of just SYNC. Here's an example test that uses an EnumSource: https://github.com/airbytehq/airbyte-platform-internal/blob/42ac3ae31fb7a3ce16862597c637ff63e8c307f1/cloud-config/cloud-config-persistence/src/test/java/io/airbyte/cloud/config/persistence/billing/BillingPersistenceTest.java#L201-L225

I guess problem is we are not doing routing logic for all TemporalJobType enums.

I think testing sync only at RouterServiceTest is fine - the other tests can be tested within taskQueueMapper logic.

Oh, the @EnumSource takes in a list of the values you want to test: https://www.baeldung.com/parameterized-tests-junit-5#3-enum

But yeah, not critical to cover everything here, I'm mostly just trying to spread awareness of ParameterizedTest because of how useful it is!

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 <[email protected]>

📝 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

reset to master

506774a

routing changes

2d39662

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

a6e4f2d

pmossman reviewed Jan 27, 2023

View reviewed changes

redo the routing service

3439654

xiaohansong requested a review from pmossman February 2, 2023 23:00

xiaohansong marked this pull request as ready for review February 2, 2023 23:00

xiaohansong requested a review from a team as a code owner February 2, 2023 23:00

Merge branch 'master' into xiaohan/disroute

9e56a1a

xiaohansong temporarily deployed to more-secrets February 2, 2023 23:02 — with GitHub Actions Inactive

xiaohansong temporarily deployed to more-secrets February 2, 2023 23:03 — with GitHub Actions Inactive

pmossman approved these changes Feb 3, 2023

View reviewed changes

add test to config repository

5b24aee

xiaohansong temporarily deployed to more-secrets February 3, 2023 19:21 — with GitHub Actions Inactive

xiaohansong added 2 commits February 6, 2023 10:17

Merge branch 'master' into xiaohan/disroute

588f1ae

query workspace not connection for discover/check

7a74738

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

Merge branch 'master' into xiaohan/disroute

2ad661d

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

remove unused bean

bb54944

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

xiaohansong merged commit 48a5554 into master Feb 7, 2023

xiaohansong deleted the xiaohan/disroute branch February 7, 2023 05:09

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

Uh oh!

Add routing logic for check and discover workflow #21822

Add routing logic for check and discover workflow #21822

Uh oh!

xiaohansong commented Jan 24, 2023 •

edited

Loading

Uh oh!

pmossman Jan 27, 2023

Uh oh!

xiaohansong Feb 2, 2023

Uh oh!

pmossman left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pmossman Feb 3, 2023

Uh oh!

xiaohansong Feb 3, 2023

Uh oh!

Uh oh!

Uh oh!

pmossman Feb 3, 2023

Uh oh!

xiaohansong Feb 3, 2023

Uh oh!

pmossman Feb 3, 2023

Uh oh!

pmossman Feb 3, 2023

Uh oh!

Uh oh!

Add routing logic for check and discover workflow #21822

Add routing logic for check and discover workflow #21822

Uh oh!

Conversation

xiaohansong commented Jan 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Recommended reading order

🚨 User Impact 🚨

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmossman left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xiaohansong commented Jan 24, 2023 •

edited

Loading