Create new tables for catalog storage #10226
Conversation
|
|
There was a problem hiding this comment.
Can you add a link to the relevant issue to the PR description body please? It makes it a bit easier as a reviewer to get context. A neat trick is that if you : closes #9896 in your PR description body , when the PR is merged, GitHub will auto-close the related issue.
This looks good to me! I'm going to add one other reviewer on it to get a second set of eyes on the migration code (I've just written less migration code than others).
| @@ -43,7 +43,8 @@ public void setup() throws Exception { | |||
| final DevDatabaseMigrator devDatabaseMigrator = new DevDatabaseMigrator(configsDatabaseMigrator); | |||
| MigrationDevHelper.runLastMigration(devDatabaseMigrator); | |||
| database.query(ctx -> ctx | |||
| .execute("TRUNCATE TABLE state, connection_operation, connection, operation, actor_oauth_parameter, actor, actor_definition, workspace")); | |||
| .execute( | |||
There was a problem hiding this comment.
is there some helper we should right for this? It seems like we have the same truncate query in a bunch of different tests and have to update it in a bunch places.
There was a problem hiding this comment.
Definitely agree with that. I haven't seen any existing helper function for that. I'll add one in BaseDatabaseConfigPersistenceTest.
There was a problem hiding this comment.
I have that ready in a separate commit.
| LOGGER.info("actor_catalog table created"); | ||
| } | ||
|
|
||
| private static void createCatalogHash(final DSLContext ctx) { |
There was a problem hiding this comment.
this variable name is a bit misleading. should it be createCatalogFetchEvent?
| private static void addConnectionTableForeignKey(final DSLContext ctx) { | ||
| final Field<UUID> sourceCatalogId = DSL.field("source_catalog_id", SQLDataType.UUID.nullable(true)); | ||
| ctx.alterTable("connection") | ||
| .add(sourceCatalogId).execute(); |
There was a problem hiding this comment.
just like we do createTableIfNotExists, I think there is a addIfNotExists for columns. I saw it reference here in jooq jOOQ/jOOQ@f586593#diff-220e645094f91672089a22c940c1f7eba42e64606422c750fb2bf19e0a22ff06R234 but I guess I'm not actually seeing it in the docs. If it possible I think we would prefer it.
There was a problem hiding this comment.
There is indeed, I'm going to use that function instead. Ideally we need to do the same for the constraint as well, I'm gonna look into it.
| create table "public"."actor_catalog"( | ||
| "id" uuid not null, | ||
| "catalog" jsonb not null, | ||
| "catalog_hash" varchar(60) not null, |
There was a problem hiding this comment.
i think it would make sense to add an index on catalog_hash. I think it is one of the common queries we will be making on this table.
| ); | ||
| create table "public"."actor_catalog_fetch_event"( | ||
| "id" uuid not null, | ||
| "actor_catalog_id" uuid not null, |
There was a problem hiding this comment.
i think it would make sense to add an index on actor_catalog_id. I think one of the queries we'll want to run frequently is when have seen this catalog returned.
| create table "public"."actor_catalog_fetch_event"( | ||
| "id" uuid not null, | ||
| "actor_catalog_id" uuid not null, | ||
| "actor_id" uuid not null, |
There was a problem hiding this comment.
I think it would make sense to do an index on actor_id. I think the most common query we will do on this table is get the record for an actor or get all records for the actor. Potentially
|
@malikdiarra here's a link to our eng handbook for how we think about draft PRs. I think you're ready to say this PR is not a draft anymore :D |
What
Add new table to persist discover schema output.
closes #9896