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.

Sign up for GitHub

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

Jump to bottom

Add back in Auto detect schema functionality in sync workflow #21361

Merged
merged 5 commits into from
Jan 17, 2023
Merged

Add back in Auto detect schema functionality in sync workflow #21361

merged 5 commits into from
Jan 17, 2023

Conversation

alovew
Copy link
Contributor

@alovew alovew commented Jan 12, 2023

Add back in Auto Detect Schema functionality that was removed due to OC issue #1210

ConfigFetchActivityImpl no longer relies on the ConfigRepository

@alovew alovew requested a review from a team as a code owner January 12, 2023 23:59
@octavia-squidington-iv octavia-squidington-iv added area/platform issues related to the platform area/worker Related to worker labels Jan 12, 2023
@alovew alovew temporarily deployed to more-secrets January 13, 2023 00:02 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 00:02 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 00:02 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 00:02 — with GitHub Actions Inactive
benmoriceau
benmoriceau reviewed Jan 13, 2023
View reviewed changes
airbyte-workers/src/main/java/io/airbyte/workers/temporal/sync/SyncWorkflowImpl.java
final int autoDetectSchemaVersion =
Workflow.getVersion(AUTO_DETECT_SCHEMA_TAG, Workflow.DEFAULT_VERSION, AUTO_DETECT_SCHEMA_VERSION);

// Temporarily disabled to address OC issue #1210
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to double check, are all the issue related to this ticket solved? and is there a way to test it in dev?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the config repository is removed from all activities (config fetch activity & refresh schema activity). I'm not sure how to test this in the data plane. @jdpgrailsdev do you know?

Copy link
Contributor

@jdpgrailsdev jdpgrailsdev Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We can at least test it locally to verify that the application starts up in "data plane" mode. This will at least test the problem we saw in production with the missing data source. Additionally, cloud now has "data plane" acceptance tests, which performs a sync in the data plane. That should also fail if there is still a problem related to this.

jdpgrailsdev
jdpgrailsdev approved these changes Jan 13, 2023
View reviewed changes
Copy link
Contributor

@jdpgrailsdev jdpgrailsdev left a comment

Choose a reason for hiding this comment

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

:shipit:

@alovew alovew temporarily deployed to more-secrets January 13, 2023 18:46 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 18:46 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 18:46 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 18:46 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 20:15 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 20:15 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 20:15 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 20:15 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

File Coverage [67.42%] 🍏
SyncWorkflowImpl.java 89.31% 🍏
ConfigFetchActivityImpl.java 70.88% 🍏
ActivityBeanFactory.java 0% ❌
Total Project Coverage 26.62% 🍏

@alovew alovew temporarily deployed to more-secrets January 13, 2023 21:11 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 21:12 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

File Coverage [67.42%] 🍏
SyncWorkflowImpl.java 89.31% 🍏
ConfigFetchActivityImpl.java 70.88% 🍏
ActivityBeanFactory.java 0% ❌
Total Project Coverage 26.62% 🍏

@alovew alovew temporarily deployed to more-secrets January 13, 2023 22:48 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 13, 2023 22:49 — with GitHub Actions Inactive
@alovew
Copy link
Contributor Author

alovew commented Jan 17, 2023

@benmoriceau @jdpgrailsdev I'm continually getting this error when I try to run this locally in data plane mode. The pods are stuck in pending, not showing errors. I'm pretty sure it's not related to these changes since i've always gotten this intermittently, but now it is happening every time.

BUILD SUCCESSFUL in 1m
265 actionable tasks: 129 executed, 128 from cache, 8 up-to-date
S3 cache 2s wasted on misses, reads: 8, elapsed: 1527ms
Finished build, proceeding with local deploy...
pod "airbyte-minio-create-bucket" force deleted
Creating jobs namespace if necessary...
Cleaning up any existing jobs pods to avoid job id conflicts...
Warning: Immediate deletion does not wait for confirmation that the running resource has been terminated. The resource may continue to run on the cluster indefinitely.
No resources found
Killing any existing processes running on desired ports...
Deploying airbyte cloud to local kube cluster...
Release "ab" does not exist. Installing it now.
Error: failed post-install: timed out waiting for the condition
Switching to docker-desktop kube context...
Switched to context "docker-desktop".
Tearing down local helm deployment...
release "ab" uninstalled
Killing port-forward of webapp proxy...

@alovew alovew requested a review from benmoriceau January 17, 2023 17:57
benmoriceau
benmoriceau requested changes Jan 17, 2023
View reviewed changes
airbyte-workers/src/main/java/io/airbyte/workers/temporal/sync/SyncWorkflowImpl.java
// return output;
// }
// }
if (autoDetectSchemaVersion >= AUTO_DETECT_SCHEMA_VERSION) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we will need a new version here. Since it wasn't commented out, the version will be in the history and even old workflow will run into undeterministic exception.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated AUTO_DETECT_SCHEMA_VERSION to 2. is that what you mean?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes that will work

@alovew alovew temporarily deployed to more-secrets January 17, 2023 18:47 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 17, 2023 18:48 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 17, 2023 18:48 — with GitHub Actions Inactive
@alovew alovew temporarily deployed to more-secrets January 17, 2023 18:48 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

Airbyte Code Coverage

File Coverage [67.42%] 🍏
SyncWorkflowImpl.java 89.31% 🍏
ConfigFetchActivityImpl.java 70.88% 🍏
ActivityBeanFactory.java 0% ❌
Total Project Coverage 26.6% 🍏

@alovew alovew requested a review from benmoriceau January 17, 2023 19:28
@alovew alovew merged commit dd0e83e into master Jan 17, 2023
@alovew alovew deleted the anne/add-back-auto-detect-schema branch January 17, 2023 20:06
@octavia-squidington-iii octavia-squidington-iii mentioned this pull request Jan 24, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benmoriceau benmoriceau benmoriceau approved these changes

@jdpgrailsdev jdpgrailsdev jdpgrailsdev approved these changes

@gosusnp gosusnp Awaiting requested review from gosusnp

@colesnodgrass colesnodgrass Awaiting requested review from colesnodgrass

Assignees
No one assigned
Labels
area/platform issues related to the platform area/worker Related to worker
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@alovew @jdpgrailsdev @benmoriceau @octavia-squidington-iv