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

Deprecate Legacy Migration System #8121

Merged
merged 1 commit into from
Nov 30, 2021
Merged

Deprecate Legacy Migration System #8121

merged 1 commit into from
Nov 30, 2021

Conversation

cgardens
Copy link
Contributor

closes #6917

What

  • Now that the we are passed the faux major version bump, we can remove the legacy migration system. This PR just removes there is called. It will focus on the least change path to remove it from the production code path. Subsequently all of the underlying code will be removed, but for now just focused on stopping it from being run and making sure tests are happy.

How

  • Remove where it is called in ServerApp.java

@github-actions github-actions bot added area/platform issues related to the platform area/server labels Nov 19, 2021
@cgardens cgardens temporarily deployed to more-secrets November 19, 2021 01:53 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 19, 2021 01:53 Inactive
benmoriceau
benmoriceau requested changes Nov 19, 2021
View reviewed changes
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.

One small cleanup comment

airbyte-server/src/main/java/io/airbyte/server/ServerApp.java Outdated

// todo (cgardens) - this method is deprecated. new migrations are not run using this code path. it
// is scheduled to be removed.
final Optional<AirbyteVersion> airbyteDatabaseVersion = runFileMigration(
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, we can remove runFileMigration as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

According to the description, this PR is trying to just remove the least amount of code possible to stop the migration from being run, so runFileMigration() will be removed in a future PR

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, I forgot the description. I checked it in IntelliJ and it is thew only place where it got called, I believe that we can remove this dead code in this review safely. IMO since it is the only place this method is call we should remove it in the same review.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't consider it as a blocker.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benmoriceau yeah. it is intentional. in case we've made a mistake and need to bring it back by removing just the call point the chance of having a complex merge conflict on revert is lower, which is why i have taken this approach. if you're okay with that can you switch your review so that gh won't block the PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

I am ok, I just hope that doing that will lead to a lot of dead code If we do it too often. As far as we make sure it is clean up, that's fine.

lmossman
lmossman approved these changes Nov 19, 2021
View reviewed changes
Copy link
Contributor

@lmossman lmossman left a comment

Choose a reason for hiding this comment

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

LGTM for a minimal change to remove the file migration call

@lmossman
Copy link
Contributor

My only concern is that it according to this slack message, it sounds like a user was able to upgrade from <0.32.0, straight to v0.32.1, which may indicate that the logic to prevent this is not working correctly? If so, we probably should not merge this change until that is fixed

@cgardens
Copy link
Contributor Author

looking into the issue that @lmossman raised before merging it.

@benmoriceau benmoriceau self-requested a review November 19, 2021 21:50
@cgardens cgardens force-pushed the cgardens/remove-file-migration branch from 3636583 to 1ebc923 Compare November 20, 2021 02:23
@cgardens cgardens changed the base branch from master to cgardens/improve-auto-migration-test November 20, 2021 02:23
@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:24 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:24 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 20, 2021 02:24 Inactive
@cgardens cgardens force-pushed the cgardens/improve-auto-migration-test branch from 2bc7fc2 to 0f43bf6 Compare November 30, 2021 00:58
Base automatically changed from cgardens/improve-auto-migration-test to master November 30, 2021 04:14
@cgardens
update test
d207118 
add env file to properties test

rename util class

add test for read resource as file

better comment on log line class

blank space

do not call run file migration from server app

clean
@cgardens cgardens force-pushed the cgardens/remove-file-migration branch from 1ebc923 to d207118 Compare November 30, 2021 04:17
@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 04:19 Inactive
@cgardens cgardens temporarily deployed to more-secrets November 30, 2021 05:37 Inactive
@cgardens
Copy link
Contributor Author

confirmed that this looked good after doing a test minor version bump here.

@cgardens cgardens merged commit c97fe68 into master Nov 30, 2021
@cgardens cgardens deleted the cgardens/remove-file-migration branch November 30, 2021 07:08
This was referenced Dec 1, 2021
Closed
Merged
@cgardens cgardens mentioned this pull request Dec 3, 2021
Merged
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
@cgardens @schlattk
Deprecate Legacy Migration System (airbytehq#8121)
8d07a74
@marcosmarxm marcosmarxm mentioned this pull request Jun 23, 2022
Merged
37 tasks
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

@lmossman lmossman lmossman approved these changes

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

Successfully merging this pull request may close these issues.

Remove legacy migration system
3 participants
@cgardens @lmossman @benmoriceau