Notifications Workflow #18735

alovew · 2022-10-31T21:11:50Z

New Connection Notifications Workflow for sending schema change notifications
Two new activities for this workflow: SlackConfigActivity and NotifySchemaChangeActivity

gosusnp

LGTM with a few side notes:

there's some missing finals that should be addressed
I am not sure about how granular we want our integration to temporal here (I left a comment in tho ConnectionNotificationWorkflowImpl. I would opt for a more high level integration, that might be a longer discussion, I am fine not blocking on this.

gosusnp · 2022-11-15T00:31:48Z

...src/main/java/io/airbyte/workers/temporal/scheduling/ConnectionNotificationWorkflowImpl.java

-  public boolean sendSchemaChangeNotification(UUID connectionId, boolean isBreaking) throws IOException, InterruptedException, ApiException {
+  public boolean sendSchemaChangeNotification(UUID connectionId)
+      throws IOException, InterruptedException, ApiException, ConfigNotFoundException, JsonValidationException {
+    StandardSync standardSync = configFetchActivity.getStandardSync(connectionId);


Suggested change

StandardSync standardSync = configFetchActivity.getStandardSync(connectionId);

final StandardSync standardSync = configFetchActivity.getStandardSync(connectionId);

Looks like final are missing in the rest of this function. You might want to double check your IDE settings.

gosusnp · 2022-11-15T00:47:24Z

...src/main/java/io/airbyte/workers/temporal/scheduling/ConnectionNotificationWorkflowImpl.java

+  private ConfigFetchActivity configFetchActivity;
+
+  @Override
+  public boolean sendSchemaChangeNotification(UUID connectionId)


This might be a style issue, we can discuss this outside of this PR if it helps.

The activity breakdown doesn't feel natural to me which brings up a few questions:

Do we really want to have one activity per API request? I find that tedious to follow, code end up very truncated, having to hop through interfaces/implementations to read. Side note, probably not a concern here due to the load of notifications, but technically, that's a few extra round trips, in terms of networking.

Looking at line 41: intuitively, I think this bit of logic should be done in the notifySchemaChangeActivity rather than in the workflow.

Yes I see what you're saying here. I'll make this a followup issue

benmoriceau · 2022-11-15T16:35:47Z

kube/resources/worker.yaml

@@ -275,6 +275,11 @@ spec:
                configMapKeyRef:
                  name: airbyte-env
                  key: USE_STREAM_CAPABLE_STATE
+            - name: SHOULD_RUN_NOTIFY_WORKFLOWS


I think that the MAX_NOTIFY_WORKERS env is missing here

* notification workflow

* wip; add micronaut * add additional json deserializer methods * wip; converting to micronaut * misc cleanup * wip; broken * wip; still broken * wip * formatting * minor code cleanup; no actual changes * wip; still broken * removed commented out code; no longer broken * wip; clean-up micronaut code * cleanup; format * fix pmd issues * remove unused file * init ApplicationTest * edited link (#19444) * move 'Example values' into intl (#19446) * Revert "Update action.yml (#19416)" (#19450) This reverts commit 78fb528. * Notifications Workflow (#18735) * notification workflow * Bmoric/remove unused code (#19188) * Tmp * Move when the deletion is performed * Re-enable disable test * PR comments * Use cancel * rename * Fix test and version check position * remove unused temporal deletion code * Remove false todo * Rm repeated test * Rm unused import * Make sure that long running activity are not retried (#19452) * Parse list of dicts in json_schema_helper.find_nodes() (#19386) * Get test on nested list/dict passing - use index to query next object for list * Fix flakecheck * Test that get_node provides correct value * Improve test and test cases * Rewrite method for better comprehension * Add test for base-level key. Rewrite method for comprehension and handling this case * adding tests * fix test * formatting * remove unused dependencies * add missing test resource * format * add missing test resource (real) * format * add back protocol-models dep * format * pr feedback; log stacktrace Co-authored-by: Sophia Wiley <106352739+sophia-wiley@users.noreply.github.com> Co-authored-by: Lake Mossman <lake@airbyte.io> Co-authored-by: Topher Lubaway <asimplechris@gmail.com> Co-authored-by: Anne <102554163+alovew@users.noreply.github.com> Co-authored-by: Benoit Moriceau <benoit@airbyte.io> Co-authored-by: Ella Rohm-Ensing <erohmensing@gmail.com>

* notification workflow

github-actions bot added area/platform issues related to the platform area/worker Related to worker labels Oct 31, 2022

alovew temporarily deployed to more-secrets October 31, 2022 22:30 Inactive

alovew temporarily deployed to more-secrets November 3, 2022 20:49 Inactive

alovew temporarily deployed to more-secrets November 3, 2022 21:17 Inactive

alovew added 10 commits November 4, 2022 14:56

new notification workflow

e74764d

add schema change notification methods

29004de

add params

d37b8f4

add functionality

e7e4f9e

add workflow

9ec0f4b

update tests

53f3514

updates

71d35e1

add wip

778becf

use api endpoint

aaff7e8

remove logs

3790d4f

alovew force-pushed the anne/notifications-workflow branch from 3747282 to 3790d4f Compare November 7, 2022 19:00

alovew temporarily deployed to more-secrets November 7, 2022 19:02 Inactive

formatting, removing loggin

cc0f1bb

alovew temporarily deployed to more-secrets November 7, 2022 19:20 Inactive

alovew temporarily deployed to more-secrets November 7, 2022 19:23 Inactive

pmd and formatting

2740661

alovew force-pushed the anne/notifications-workflow branch from ebecf5e to 2740661 Compare November 7, 2022 20:11

Merge branch 'master' into anne/notifications-workflow

3dd1f7c

alovew temporarily deployed to more-secrets November 7, 2022 20:13 Inactive

alovew temporarily deployed to more-secrets November 7, 2022 21:01 Inactive

alovew temporarily deployed to more-secrets November 8, 2022 00:18 Inactive

alovew marked this pull request as ready for review November 8, 2022 00:34

Merge branch 'master' into anne/notifications-workflow

08a2325

alovew temporarily deployed to more-secrets November 8, 2022 01:16 Inactive

Merge branch 'master' into anne/notifications-workflow

584a9f6

alovew temporarily deployed to more-secrets November 14, 2022 23:55 Inactive

formatting

f1224c7

alovew temporarily deployed to more-secrets November 15, 2022 00:24 Inactive

alovew temporarily deployed to more-secrets November 15, 2022 00:30 Inactive

fix test

acb5ab5

alovew force-pushed the anne/notifications-workflow branch from e1a50d3 to acb5ab5 Compare November 15, 2022 00:50

gosusnp approved these changes Nov 15, 2022

View reviewed changes

alovew temporarily deployed to more-secrets November 15, 2022 00:52 Inactive

add finals

1dbfb64

alovew temporarily deployed to more-secrets November 15, 2022 01:12 Inactive

format

a4d68b1

alovew temporarily deployed to more-secrets November 15, 2022 01:41 Inactive

alovew temporarily deployed to more-secrets November 15, 2022 02:36 Inactive

alovew temporarily deployed to more-secrets November 15, 2022 03:35 Inactive

benmoriceau reviewed Nov 15, 2022

View reviewed changes

add max notify workers

6f4c250

alovew temporarily deployed to more-secrets November 15, 2022 18:08 Inactive

benmoriceau approved these changes Nov 15, 2022

View reviewed changes

alovew temporarily deployed to more-secrets November 15, 2022 19:08 Inactive

alovew added 2 commits November 15, 2022 12:49

Merge branch 'master' into anne/notifications-workflow

a32c0e3

Merge branch 'master' into anne/notifications-workflow

751492a

alovew temporarily deployed to more-secrets November 15, 2022 21:14 Inactive

alovew merged commit fdb96d0 into master Nov 15, 2022

alovew deleted the anne/notifications-workflow branch November 15, 2022 21:58

github-actions bot mentioned this pull request Nov 15, 2022

Bump helm chart version reference to 0.40.43 #19453

Closed

colesnodgrass pushed a commit that referenced this pull request Nov 16, 2022

Notifications Workflow (#18735)

27790af

* notification workflow

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

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

akashkulk pushed a commit that referenced this pull request Dec 2, 2022

Notifications Workflow (#18735)

0f6f303

* notification workflow

edmundito mentioned this pull request Dec 13, 2022

Update settings to toggle auto-detect schema changes notifications #17830

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications Workflow #18735

Notifications Workflow #18735

alovew commented Oct 31, 2022 •

edited

Loading

gosusnp left a comment

gosusnp Nov 15, 2022

gosusnp Nov 15, 2022

gosusnp Nov 15, 2022

alovew Nov 15, 2022

benmoriceau Nov 15, 2022

	StandardSync standardSync = configFetchActivity.getStandardSync(connectionId);
	final StandardSync standardSync = configFetchActivity.getStandardSync(connectionId);

Notifications Workflow #18735

Notifications Workflow #18735

Conversation

alovew commented Oct 31, 2022 • edited Loading

gosusnp left a comment

Choose a reason for hiding this comment

gosusnp Nov 15, 2022

Choose a reason for hiding this comment

gosusnp Nov 15, 2022

Choose a reason for hiding this comment

gosusnp Nov 15, 2022

Choose a reason for hiding this comment

alovew Nov 15, 2022

Choose a reason for hiding this comment

benmoriceau Nov 15, 2022

Choose a reason for hiding this comment

alovew commented Oct 31, 2022 •

edited

Loading