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: refresh actor configuration and state between sync attempts #21629

Merged
merged 30 commits into from
Feb 1, 2023

Conversation

pedroslopez
Copy link
Contributor

@pedroslopez pedroslopez commented Jan 19, 2023

What

This is part 1 of #20912, focused on removing the actor configuration and state from JobSyncConfig and moving into a new AttemptSyncConfig at the Attempt level. It will also mean that config/state is fetched when building the sync input rather than retrieving it from the job's JobSyncConig.

Two issues are fixed with these changes:

  1. Refreshing state between sync attempts (fixes checkpointing! - closes Airbyte does not use state from last attempt #17774)
  2. Refreshing actor configuration between sync attempts (part of Refresh connector configuration between attempts / activities #20912)

However, it does not address when the CHECK that can run before the SyncWorkflow makes changes to the configuration. This will be addressed via a follow up PR (part 2) via the solutions outlined in the tech spec for #20912, specifically the part on changing the ConnectionManagerWorkflow activities that generate inputs.

How

  • Add new AttemptSyncConfig pojo with sourceConfiguration destinationConfiguration and state fields.
  • Remove above fields from JobSyncConfig
  • Add attempt_sync_config column to the attempts table.
  • Build AttemptSyncConfig when generating the sync inputs by fetching the latest actor configuration + state, and write the values to the above column.
  • Introduce a new internal api endpoint to update attempt sync config
  • Update references that used the JobSyncConfig to instead use the values from AttemptSyncConfig

@octavia-squidington-iv octavia-squidington-iv added area/documentation Improvements or additions to documentation area/platform issues related to the platform area/worker Related to worker labels Jan 19, 2023
@pedroslopez pedroslopez temporarily deployed to more-secrets January 19, 2023 23:56 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 19, 2023 23:56 — with GitHub Actions Inactive
@octavia-squidington-iv octavia-squidington-iv removed the area/documentation Improvements or additions to documentation label Jan 20, 2023
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:05 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:05 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:36 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:36 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:50 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 00:50 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 16:10 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 16:10 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 19:25 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 19:25 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Jan 20, 2023

Airbyte Code Coverage

File Coverage [78.94%] 🍏
V0_40_28_001__AddAttemptSyncConfig.java 100% 🍏
DefaultJobCreator.java 100% 🍏
DefaultJobPersistence.java 95.62% 🍏
JobTracker.java 94.7% 🍏
GenerateInputActivityImpl.java 86.04% 🍏
Attempt.java 85.65% 🍏
Job.java 84.87% 🍏
JobPersistence.java 80% 🍏
TemporalWorkerRunFactory.java 77.64% 🍏
TemporalClient.java 61.44% 🍏
ApiClientBeanFactory.java 0%
ProtocolConverters.java 0%
StateConverter.java 0%
AttemptApiController.java 0%
ApplicationBeanFactory.java 0%
Total Project Coverage 24.62%

@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 20:16 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets January 20, 2023 20:16 — with GitHub Actions Inactive
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.

Sorry for the delay, I missed this PR. It LGTM, it doesn't seems that there is a concurrency issue with this PR. About the comment where you tagged me, I believe that it is some legacy code that should be removed. This shouldn't be part of your PR and I'll create an issue for for that.

@pedroslopez pedroslopez temporarily deployed to more-secrets February 1, 2023 07:56 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 1, 2023 07:56 — with GitHub Actions Inactive
@pedroslopez pedroslopez enabled auto-merge (squash) February 1, 2023 14:09
@pedroslopez pedroslopez temporarily deployed to more-secrets February 1, 2023 14:10 — with GitHub Actions Inactive
@pedroslopez pedroslopez temporarily deployed to more-secrets February 1, 2023 14:10 — with GitHub Actions Inactive
@pedroslopez pedroslopez merged commit d22dfe8 into master Feb 1, 2023
@pedroslopez pedroslopez deleted the pedroslopez/attempt-sync-config branch February 1, 2023 14:47
pedroslopez added a commit that referenced this pull request Feb 1, 2023
pedroslopez added a commit that referenced this pull request Feb 1, 2023
pedroslopez added a commit that referenced this pull request Feb 2, 2023
pedroslopez added a commit that referenced this pull request Feb 7, 2023
…nc attempts"" (#22281)

* Revert "Revert "fix: refresh actor configuration and state between sync attempts (#21629)" (#22211)"

This reverts commit 7978862.

* fmt
danidelvalle pushed a commit to danidelvalle/airbyte that referenced this pull request Feb 9, 2023
…nc attempts"" (airbytehq#22281)

* Revert "Revert "fix: refresh actor configuration and state between sync attempts (airbytehq#21629)" (airbytehq#22211)"

This reverts commit 7978862.

* fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server area/worker Related to worker from/connector-ops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Airbyte does not use state from last attempt
5 participants