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

fix(platform): use updated configuration after check during syncs #91

Merged
merged 7 commits into from
Feb 23, 2023

Conversation

pedroslopez
Copy link
Contributor

What

Migrated from airbytehq/airbyte#21899

fixes airbytehq/airbyte#20912

This PR builds on the changes introduced in airbytehq/airbyte#21629. While the aforementioned PR addressed using updated state/config values between attempts, this PR solves the issue of updating configuration within an attempt. This can happen when, for example, an oauth token is refreshed during the check operation that can run prior to the SyncWorkflow.

How

This implements the second part of the solution as laid out in the tech spec, changing when we're generating the sync input and how we are generating the check inputs.

Before this change, the SyncWorkflow input was being generated at the start of the ConnectionManagerWorkflow, and was then converted into Check inputs if needed. The activities look something like this:

image

(these are snippets of diagrams, the full views can be found in the tech spec)

After this change, we separate the check input generation into its own activity method. Then, we can move the SyncWorkflow input generation to after the check activity runs. Because the previous PR updated the SyncWorkflow input generation activity to fetch latest configs, this will take into consideration any changes that happened during check.

This ends up looking something like this:

image

The change is versioned, so ConnectionManagerWorkflows that executed before this change will still follow the old path/behavior.

🚨 User Impact 🚨

No breaking change. Configs updated during check will now correctly be retrieved for the sync.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 21, 2023

Airbyte Code Coverage

File Coverage [77.73%] 🍏
FeatureFlagFetchActivityImpl.java 90.54% 🍏
GenerateInputActivityImpl.java 89.21% 🍏
SyncCheckConnectionResult.java 87.79% 🍏
ConnectionManagerWorkflowImpl.java 85.75% 🍏
GenerateInputActivity.java 43.02%
Total Project Coverage 26.96% 🍏

@pedroslopez
Copy link
Contributor Author

This PR is pending an addition of a feature flag using the changes from #79 , but reviewers, please review away!

Copy link
Contributor

@gosusnp gosusnp left a comment

Choose a reason for hiding this comment

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

Looks pretty close, I think we have some code duplication in how we build inputs, wondering if we could avoid some of it.

Copy link
Contributor

@benmoriceau benmoriceau left a comment

Choose a reason for hiding this comment

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

LGTM, Letting @gosusnp to approve

@pedroslopez
Copy link
Contributor Author

I've now added the feature flag! connectionManagerWorkflow.checkInputGeneration will default to false and keep the old ordering of activities / generate check input from the sync input as was done previously, but when enabled will run the new activity for checks and reorder the sync input generation activity to after the checks.

We'll start by rolling this out to some test and later internal workspaces, followed by a full rollout once that's validated after one week. After seeing this live for everyone for another week we'll come back and remove the feature flag.

@github-actions
Copy link
Contributor

github-actions bot commented Feb 23, 2023

Platform Test Results

   240 files  ±0     240 suites  ±0   22m 36s ⏱️ - 1m 29s
1 667 tests +4  1 656 ✔️ +4  11 💤 ±0  0 ±0 
1 686 runs  +4  1 675 ✔️ +4  11 💤 ±0  0 ±0 

Results for commit 7d73004. ± Comparison against base commit 0b4e741.

♻️ This comment has been updated with latest results.

@gosusnp
Copy link
Contributor

gosusnp commented Feb 23, 2023

LGTM

@benmoriceau
Copy link
Contributor

/approve-and-merge reason="looks safe"

@octavia-approvington
Copy link
Contributor

A crack team of mammals has made a decision.
imagine a seal of approval

@octavia-approvington octavia-approvington merged commit 484cc85 into main Feb 23, 2023
@octavia-approvington octavia-approvington deleted the pedroslopez/check-input-gen branch February 23, 2023 23:43
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.

Refresh connector configuration between attempts / activities
5 participants