implement flyway migration for config database normalization + tests #8563

subodh1810 · 2021-12-06T23:07:10Z

Issue : #8438

🚨 User Impact 🚨

Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

New Connector

Community member or Airbyter

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the connector is published, connector added to connector index as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Updating a connector

Community member or Airbyter

Grant edit access to maintainers (instructions)
Secrets in the connector's spec are annotated with airbyte_secret
Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
Code reviews completed
Documentation updated
- Connector's README.md
- Connector's bootstrap.md. See description and examples
- Changelog updated in docs/integrations/<source or destination>/<name>.md including changelog. See changelog example
PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

Create a non-forked branch based on this PR and test the below items on it
Build is successful
Credentials added to Github CI. Instructions.
/test connector=connectors/<name> command is passing.
New Connector version released on Dockerhub by running the /publish command described here
After the new connector version is published, connector version bumped in the seed directory as described here
Seed specs have been re-generated by building the platform and committing the changes to the seed spec files, as described here

Connector Generator

Issue acceptance criteria met
PR name follows PR naming conventions
If adding a new generator, add it to the list of scaffold modules being tested
The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
Documentation which references the generator is updated as needed.

cgardens

I noted a couple minor alterations to the schema, but once those are done, I think you are good to go! Very nice!

cgardens · 2021-12-07T05:52:04Z

airbyte-commons/src/main/java/io/airbyte/commons/json/Jsons.java

@@ -49,6 +49,10 @@
    }
  }

+  public static <T> T convertValue(final Object object, final Class<T> klass) {


can you add a javadoc comment explaining what context this should be used in.

cgardens · 2021-12-07T05:59:06Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  "name" varchar(256) not null,
+  "configuration" jsonb not null,
+  "actor_type" actor_type not null,
+  "tombstone" bool not null,


for all of the tombstones, can we set a default value of false here? when we create these objects the common case is that it is not tombstoned yet.

cgardens · 2021-12-07T05:59:57Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  "name" varchar(256) not null,
+  "docker_repository" varchar(256) not null,
+  "docker_image_tag" varchar(256) not null,
+  "documentation_url" varchar(256) not null,


we should allow this one to be null.

cgardens · 2021-12-07T06:01:06Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  "updated_at" timestamptz(35) not null,
+  constraint "actor_oauth_parameter_pkey"
+    primary key ("id")
+);
 create table "public"."airbyte_configs"(


after this migration we can drop this table, right? totally fine if we don't do it in this migration.

Yeah I was thinking cleaning up the table as part of a separate migration. I thought it would be more safe

cgardens · 2021-12-07T06:03:03Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  "configuration" jsonb not null,
+  "actor_type" actor_type not null,
+  "tombstone" bool not null,
+  "created_at" timestamptz(35) not null,


for all of the created_at and updated_at can we have it default to the current timestamp, please? setting that default helps avoid a bunch of manual input that would otherwise just be the current timestamp anyway.

cgardens · 2021-12-07T06:11:10Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+create unique index "state_pkey" on "public"."state"(
+  "id" asc, 
+  "connection_id" asc
+);


connection_operation is missing in this schema_dump for some reason.

cgardens · 2021-12-07T06:12:33Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+    foreign key ("connection_id")
+    references "public"."connection" ("id");
+create index "actor_actor_definition_id_idx" on "public"."actor"("actor_definition_id" asc);
+create index "actor_actor_type_idx" on "public"."actor"("actor_type" asc);


i'm not sure how to think about this index. it's just going to bucket into one of two categories? what query is this making more efficient?

same for the other two actor_type indexes. i don't think they will usually get used. usually for really low cardinality fields it doesn't make sense to do an index. this SO article does an okay job explaining why.

cgardens · 2021-12-07T06:18:03Z

...irbyte/db/instance/configs/migrations/V0_32_8_001__AirbyteConfigDatabaseDenormalization.java

+          .set(createdAt, OffsetDateTime.ofInstant(configWithMetadata.getCreatedAt(), ZoneOffset.UTC))
+          .set(updatedAt, OffsetDateTime.ofInstant(configWithMetadata.getUpdatedAt(), ZoneOffset.UTC))
+          .execute();
+      createAndPopulateConnectionOperation(ctx, !connectionOperationCreated, configWithMetadata);


ah here's the problem. in the schema_dump i noticed this table doesn't exist. it is because it is never created because the creation of the table is happening in here. those two concerns need to be separated. the table should be created, even if there are no operations in the initial migration.

cgardens · 2021-12-07T06:19:06Z

...irbyte/db/instance/configs/migrations/V0_32_8_001__AirbyteConfigDatabaseDenormalization.java

+    final Field<OffsetDateTime> createdAt = DSL.field("created_at", SQLDataType.TIMESTAMPWITHTIMEZONE.nullable(false));
+    final Field<OffsetDateTime> updatedAt = DSL.field("updated_at", SQLDataType.TIMESTAMPWITHTIMEZONE.nullable(false));
+    final Field<JSONB> configBlob = DSL.field("config_blob", SQLDataType.JSONB.nullable(false));
+    final Result<Record> results = ctx.select(asterisk()).from(DSL.table("airbyte_configs")).where(configType.eq(airbyteConfigType.name())).fetch();


worth noting that we are making an assumption here that each table we are pulling can fully fit in memory.

do you think it might cause problems and needs change?

that's a great point charles, it might if the database has a ton of connections, sources or destinations.

I'll be curious to test this. I think we should test how this performs with a database with ten thousands connections to be safe. I don't expect the other resources to be used with the same level of scale.

cgardens · 2021-12-07T06:19:51Z

...irbyte/db/instance/configs/migrations/V0_32_8_001__AirbyteConfigDatabaseDenormalization.java

+    createAndPopulateState(ctx);
+  }
+
+  private static void createEnums(final DSLContext ctx) {


i don't think i saw the enums in the schema dump fwiw.

for some reason the automatic schema dump command ./gradlew :airbyte-db:lib:dumpConfigsSchema doesnt create the enums. Added them manually

any luck on getting these to populate automatically?

davinchia · 2021-12-10T16:47:00Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  constraint "actor_definition_pkey"
+    primary key ("id")
+);
+create table "public"."actor_oauth_parameter"(


did you figure out how to get jooq to add enums to the schema dump?

cgardens · 2021-12-15T04:51:01Z

airbyte-db/lib/src/main/resources/configs_database/schema_dump.txt

+  "schedule" jsonb null,
+  "manual" bool not null,
+  "resource_requirements" jsonb null,
+  "created_at" timestamptz(35) not null default cast(current_timestamp as timestamp with time zone),


nice work on getting this default to populate!

* implement new database config persistence * make new persistence compatible with applications * update tests * do not update createdAt timestamp * review comments + incorporate changes from bootloader * address review comments * fixed test + remove unused method * fix archive handler test * handle null value for tombstone * add logs for assert migration * final review comments

implement flyway migration for config database normalization + tests

7215493

subodh1810 self-assigned this Dec 6, 2021

subodh1810 temporarily deployed to more-secrets December 6, 2021 23:08 Inactive

cgardens approved these changes Dec 7, 2021

View reviewed changes

subodh1810 added 2 commits December 8, 2021 02:52

use Enums + create connection operation table

6316eb9

address review comments

4b515e0

subodh1810 temporarily deployed to more-secrets December 7, 2021 23:27 Inactive

format

12bb192

subodh1810 temporarily deployed to more-secrets December 7, 2021 23:34 Inactive

undo change

887cf24

subodh1810 temporarily deployed to more-secrets December 7, 2021 23:36 Inactive

subodh1810 requested review from davinchia and tuliren December 9, 2021 00:39

davinchia reviewed Dec 10, 2021

View reviewed changes

davinchia approved these changes Dec 10, 2021

View reviewed changes

Merge branch 'master' into config-database-migration-implementation

a3a4a0c

subodh1810 temporarily deployed to more-secrets December 12, 2021 19:05 Inactive

subodh1810 added 2 commits December 14, 2021 22:52

Merge branch 'master' into config-database-migration-implementation

1710ef7

handle null value for enums

788f918

subodh1810 temporarily deployed to more-secrets December 14, 2021 17:29 Inactive

cgardens reviewed Dec 15, 2021

View reviewed changes

Merge branch 'master' into config-database-migration-implementation

078094b

subodh1810 temporarily deployed to more-secrets December 15, 2021 09:41 Inactive

Merge branch 'master' into config-database-migration-implementation

6801c3b

subodh1810 temporarily deployed to more-secrets December 21, 2021 10:25 Inactive

github-actions bot added area/platform issues related to the platform area/server labels Dec 21, 2021

subodh1810 temporarily deployed to more-secrets December 21, 2021 13:00 Inactive

subodh1810 merged commit 8654c4a into master Dec 21, 2021

subodh1810 deleted the config-database-migration-implementation branch December 21, 2021 17:03

jrhizor mentioned this pull request Dec 22, 2021

Bump Airbyte version from 0.34.4-alpha to 0.35.0-alpha #9023

Merged

pmossman mentioned this pull request Feb 1, 2022

Source definition deletion silently fails #9912

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement flyway migration for config database normalization + tests #8563

implement flyway migration for config database normalization + tests #8563

subodh1810 commented Dec 6, 2021

cgardens left a comment •

edited

cgardens Dec 7, 2021

davinchia Dec 10, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

subodh1810 Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

cgardens Dec 7, 2021

subodh1810 Dec 7, 2021

davinchia Dec 10, 2021

cgardens Dec 7, 2021

subodh1810 Dec 7, 2021

cgardens Dec 15, 2021

davinchia Dec 10, 2021 •

edited

cgardens Dec 15, 2021 •

edited

implement flyway migration for config database normalization + tests #8563

implement flyway migration for config database normalization + tests #8563

Conversation

subodh1810 commented Dec 6, 2021

🚨 User Impact 🚨

Pre-merge Checklist

Community member or Airbyter

Airbyter

Community member or Airbyter

Airbyter

cgardens left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davinchia Dec 10, 2021 • edited

Choose a reason for hiding this comment

cgardens Dec 15, 2021 • edited

Choose a reason for hiding this comment

cgardens left a comment •

edited

davinchia Dec 10, 2021 •

edited

cgardens Dec 15, 2021 •

edited