-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add tombstone field to actor_definitions table #9988
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the schema of the table in the file
airbyte-db/lib/src/main/resources/configs_database/normalized_tables_schema.txt
Also once you merge this, when you make a release, you will have to upgrade the CONFIGS_DATABASE_MINIMUM_FLYWAY_MIGRATION_VERSION
parameter in all the places to the version introduced.
When this would be released to cloud, again CONFIGS_DATABASE_MINIMUM_FLYWAY_MIGRATION_VERSION
parameter would need to be updated before deploying as part of the upgrade OSS version PR.
This is manual right now
public class V0_35_14_001__AddTombstoneToActorDefinitionTest extends AbstractConfigsDatabaseTest { | ||
|
||
@Test | ||
public void test() throws SQLException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we populate the table before hand with a random record and then run the migration and then assert the value of the new column to make sure the not null constraint is being respected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, added this!
@@ -77,6 +77,7 @@ Referenced by: | |||
spec | jsonb | | not null | | |||
created_at | timestamp with time zone | | not null | CURRENT_TIMESTAMP | |||
updated_at | timestamp with time zone | | not null | CURRENT_TIMESTAMP | |||
tombstone | boolean | | not null | false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subodh1810 added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Remember to update the CONFIGS_DATABASE_MINIMUM_FLYWAY_MIGRATION_VERSION
at all the places and when you deploy this to cloud, there as well
2a77295
to
eed18bb
Compare
eed18bb
to
597a7d6
Compare
What
#9912 was opened because connector definition deletions via API were not persisting. I added the ability to delete connector definitions while the database denormalization project was ongoing, and the new 'tombstone' field did not make it into the actor_definitions table.
This PR adds a migration to add the tombstone column, and updates configPersistence to write handle the new column.
How
馃毃 User Impact 馃毃
This should fix connector definition deletions