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

Go back v0 as default #22278

Merged
merged 9 commits into from
Feb 2, 2023
Merged

Conversation

gosusnp
Copy link
Contributor

@gosusnp gosusnp commented Feb 1, 2023

What

Addresses complications with upgrading the platform and normalization to support V1 protocol changes. This change will focus on making sure that platform is downgrading V1 protocols on-the-fly to ensure the catalog stored is V0. This effort will need to be in coordination with the normalization being downgraded to support V0 protocol

How

Adjust changes that were made to update the protocol version to V1 to go back and support V0 protocol until the migration process can be handled

Recommended reading order

  1. x.java
  2. y.python

* update config to specify v0 as max protocol version
* revert back v0 ser/de to use the objects from the unversioned
  namespace
* disable v1 migrations
* fix the default to allow MigrationContainer to run without a migration
@gosusnp gosusnp requested review from edgao and ryankfu February 1, 2023 23:53
@octavia-squidington-iii octavia-squidington-iii added area/platform issues related to the platform area/server area/worker Related to worker labels Feb 1, 2023
@gosusnp gosusnp temporarily deployed to more-secrets February 1, 2023 23:55 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 1, 2023 23:55 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 1, 2023 23:58 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Platform Test Results

   239 files   -   4     239 suites   - 4   12m 7s ⏱️ - 12m 10s
1 626 tests  - 37  1 620 ✔️  - 32  5 💤  - 6  1 +1 
1 641 runs   - 41  1 635 ✔️  - 36  5 💤  - 6  1 +1 

For more details on these failures, see this check.

Results for commit c0eedfe. ± Comparison against base commit 6d65070.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@ryankfu ryankfu left a comment

Choose a reason for hiding this comment

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

LGTM

@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:09 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:09 — with GitHub Actions Inactive
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

one nit on a comment. Code changes look right. I don't feel qualified to say if there were any files that should have been changed, that were missed though.

}

/**
* Performs of search of a v0 data type node, returns true at the first node found.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Performs of search of a v0 data type node, returns true at the first node found.
* Performs of search of a v1 data type node, returns true at the first node found.

@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:11 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:41 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:41 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 00:41 — with GitHub Actions Inactive
@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Airbyte Code Coverage

File Coverage [61.94%] 🍏
ConfiguredAirbyteCatalogMigrationV1.java 100% 🍏
AirbyteMessageV0Deserializer.java 100% 🍏
AirbyteMessageV0Serializer.java 100% 🍏
AirbyteMessageMigrationV1.java 97.56% 🍏
SchemaMigrationV1.java 96.39% 🍏
DefaultJobPersistence.java 95.54% 🍏
MigrationContainer.java 94.74% 🍏
DbConverter.java 79.86% 🍏
EnvConfigs.java 45.75%
NormalizationActivityImpl.java 9.59%
CatalogMigrationV1Helper.java 0%
AirbyteAcceptanceTestHarness.java 0%
Total Project Coverage 24.51%

Copy link
Member

@colesnodgrass colesnodgrass left a comment

Choose a reason for hiding this comment

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

Changes seem fine. What's the confidence that the downgrade code works as expected?

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2023

Kube Test Results

  47 files  ±0    47 suites  ±0   9m 4s ⏱️ +39s
208 tests ±0  172 ✔️ ±0  36 💤 ±0  0 ±0 
226 runs  ±0  190 ✔️ ±0  36 💤 ±0  0 ±0 

Results for commit c0eedfe. ± Comparison against base commit 6d65070.

♻️ This comment has been updated with latest results.

@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 01:48 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 01:48 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 02:17 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 02:17 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 03:07 — with GitHub Actions Inactive
@gosusnp gosusnp temporarily deployed to more-secrets February 2, 2023 03:07 — with GitHub Actions Inactive
@gosusnp
Copy link
Contributor Author

gosusnp commented Feb 2, 2023

/approve-and-merge reason=“Merging to unblock downtream steps, will keep an eye on the result"

@octavia-approvington
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform area/server area/worker Related to worker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants