Disable all broken connections when source is refreshed #20208

alovew · 2022-12-07T22:21:18Z

When one connection's source is refreshed, we want to check each connection that uses that same source and disable them where necessary.

Changes are:

new ConfigRepository and ConnectionHandler methods to fetch all connections for a particular source id
updates in the SchedulerHandler to iterate through all connections for a particular source and check their diffs

gosusnp · 2022-12-08T22:50:07Z

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

@@ -865,6 +865,20 @@ public List<StandardSync> listWorkspaceStandardSyncs(final UUID workspaceId, fin
    return getStandardSyncsFromResult(connectionAndOperationIdsResult);
  }

+  public List<StandardSync> listConnectionsBySource(final UUID sourceId, final boolean includeDeleted) throws IOException {


I just put up a PR with a more complete version of this: https://github.com/airbytehq/airbyte/pull/20264/files#diff-2d29b293778e290157be96e22294c2df1fdfa7c72a48961d03396776446deeddR852
However, my PR might take longer to merge due to breaking UI changes.

gosusnp · 2022-12-08T22:52:22Z

airbyte-server/src/main/java/io/airbyte/server/handlers/SchedulerHandler.java

-      connectionStatus = ConnectionStatus.INACTIVE;
-    } else {
-      connectionStatus = connectionRead.getStatus();
+    final ConnectionReadList connectionsForSource = connectionsHandler.listConnectionsForSource(discoverSchemaRequestBody.getSourceId(), false);


I would advocate for avoiding having handlers depend on other handlers. At the handler level, I am currently leaning towards using our DB abstraction directly until we introduce a better layer.
It might be worth checking with the team to agree on a direction moving forward here.

oh interesting, you mean use configPersistence directly instead of going through the connection handler? I may have misunderstood how we wanted to migrate this

Yes and I don't think we agreed on a strategy here.

gosusnp · 2022-12-08T22:55:25Z

airbyte-server/src/main/java/io/airbyte/server/handlers/SchedulerHandler.java

+        connectionStatus = connectionRead.getStatus();
+      }
+      updateObject.status(connectionStatus);
+      connectionsHandler.updateConnection(updateObject);


This feels very broad, from this function, I expect we only modify syncCatalog and the breaking change flag, but this function will actually override the whole object. Should we introduce a more scoped update here?

This is actually a patch update even though the object is named ConnectionUpdate, so it is only modifying the breaking change field and status

Yes, my point is that if we add other fields to the updateObject we could end up overriding fields by mistake.
I don't think it is a big issue currently, but this could lead to some surprises.

you mean adding other fields within this method here? we would only be updating other fields here if we explicitly stated them on the ConnectionUpdate object, but I think that would be expected

gosusnp · 2022-12-08T22:56:26Z

airbyte-server/src/main/java/io/airbyte/server/handlers/SchedulerHandler.java

+      updateObject.status(connectionStatus);
+      connectionsHandler.updateConnection(updateObject);
+      if (connectionRead.getConnectionId().equals(discoverSchemaRequestBody.getConnectionId())) {
+        discoveredSchema.catalogDiff(diff).breakingChange(containsBreakingChange).connectionStatus(connectionStatus);


Can we add some description to the function to highlight that we are modifying the input?

just added a comment where we're calling the function - do you think that works?

I'd say it should be part of the documentation of this function.

gosusnp · 2022-12-08T23:50:11Z

airbyte-server/src/main/java/io/airbyte/server/handlers/SchedulerHandler.java

+      updateObject.status(connectionStatus);
+      connectionsHandler.updateConnection(updateObject);
+      if (connectionRead.getConnectionId().equals(discoverSchemaRequestBody.getConnectionId())) {
+        discoveredSchema.catalogDiff(diff).breakingChange(containsBreakingChange).connectionStatus(connectionStatus);


I'd say it should be part of the documentation of this function.

alovew requested a review from a team as a code owner December 7, 2022 22:21

octavia-squidington-iv added area/platform issues related to the platform area/server area/worker Related to worker labels Dec 7, 2022

alovew temporarily deployed to more-secrets December 7, 2022 22:23 — with GitHub Actions Inactive

alovew force-pushed the anne/pause-all-connections-with-source branch from e8cc8ce to fcb37b8 Compare December 7, 2022 23:28

octavia-squidington-iv added the area/documentation Improvements or additions to documentation label Dec 7, 2022

alovew temporarily deployed to more-secrets December 7, 2022 23:30 — with GitHub Actions Inactive

alovew added 18 commits December 8, 2022 12:03

refresh before syncs when feature flag is on

26bc09d

tests

ac4c0a4

fix activity

9ee3347

inject envvariablefeatureflags

8c2901e

inject envvariable feature flags

5aad0e9

remove logging

12d8d90

inject into activities instead of workflow

4a63b49

remove from connection manager workflow

7b7724d

remove in tests

6d46a4d

try catch

8703218

auto detect schema version

cd57cd5

Catch errors

e724939

tests

c5019d7

update tests

c4f2c6d

pause all connections with same source

d655bfa

tests

5e8a5fe

updates

ca2cda3

test

8432701

alovew force-pushed the anne/pause-all-connections-with-source branch from fcb37b8 to 8432701 Compare December 8, 2022 21:08

octavia-squidington-iv removed the area/documentation Improvements or additions to documentation label Dec 8, 2022

alovew temporarily deployed to more-secrets December 8, 2022 21:10 — with GitHub Actions Inactive

alovew temporarily deployed to more-secrets December 8, 2022 21:11 — with GitHub Actions Inactive

alovew added 2 commits December 8, 2022 13:22

remove old changes from rebase

9c9e01d

one more removal

83567c9

alovew requested review from benmoriceau, gosusnp, jdpgrailsdev and colesnodgrass December 8, 2022 21:24

alovew temporarily deployed to more-secrets December 8, 2022 21:25 — with GitHub Actions Inactive

gosusnp reviewed Dec 8, 2022

View reviewed changes

add comment about function

3b52950

alovew temporarily deployed to more-secrets December 8, 2022 23:40 — with GitHub Actions Inactive

gosusnp approved these changes Dec 8, 2022

View reviewed changes

add comment on function

46e876c

alovew temporarily deployed to more-secrets December 9, 2022 00:43 — with GitHub Actions Inactive

alovew temporarily deployed to more-secrets December 9, 2022 00:44 — with GitHub Actions Inactive

alovew merged commit 0a8fc9e into master Dec 9, 2022

alovew deleted the anne/pause-all-connections-with-source branch December 9, 2022 18:36

github-actions bot mentioned this pull request Dec 9, 2022

Bump helm chart version reference to 0.43.2 #20320

Closed

octavia-squidington-iii mentioned this pull request Dec 18, 2022

Bump Airbyte version from 0.40.25 to 0.40.26 #20630

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable all broken connections when source is refreshed #20208

Disable all broken connections when source is refreshed #20208

alovew commented Dec 7, 2022 •

edited

Loading

gosusnp Dec 8, 2022

gosusnp Dec 8, 2022

alovew Dec 8, 2022

gosusnp Dec 8, 2022

gosusnp Dec 8, 2022

alovew Dec 8, 2022

gosusnp Dec 8, 2022

alovew Dec 8, 2022

gosusnp Dec 8, 2022

alovew Dec 8, 2022

gosusnp Dec 8, 2022

gosusnp Dec 8, 2022

Disable all broken connections when source is refreshed #20208

Disable all broken connections when source is refreshed #20208

Conversation

alovew commented Dec 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alovew commented Dec 7, 2022 •

edited

Loading