-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Add persistence function for discovered schema #10326
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
Conversation
cbbceeb
to
669c9f4
Compare
669c9f4
to
7863845
Compare
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.
Looks good in general. Could you add more unit tests for the list-
methods in DatabaseConfigPersistence
?
...onfig/persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java
Outdated
Show resolved
Hide resolved
...onfig/persistence/src/main/java/io/airbyte/config/persistence/DatabaseConfigPersistence.java
Outdated
Show resolved
Hide resolved
.../io/airbyte/db/instance/configs/migrations/V0_35_28_001__AddActorCatalogMetadataColumns.java
Outdated
Show resolved
Hide resolved
.../io/airbyte/db/instance/configs/migrations/V0_35_28_001__AddActorCatalogMetadataColumns.java
Outdated
Show resolved
Hide resolved
…sistence/DatabaseConfigPersistence.java Co-authored-by: LiRen Tu <[email protected]>
…sistence/DatabaseConfigPersistence.java Co-authored-by: LiRen Tu <[email protected]>
…grations/V0_35_28_001__AddActorCatalogMetadataColumns.java Co-authored-by: LiRen Tu <[email protected]>
…grations/V0_35_28_001__AddActorCatalogMetadataColumns.java Co-authored-by: LiRen Tu <[email protected]>
This is done! |
.from(ACTOR_CATALOG_FETCH_EVENT) | ||
.where(ACTOR_CATALOG_FETCH_EVENT.ID.eq(actorCatalogFetchEvent.getId()))); | ||
|
||
if (!isExistingConfig) { |
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.
Shouldnt we have an update feature like other configs?
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.
Those table are used to cache information, so insert and get should really be the only access patterns. I can add the update case for consistency though
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.
I added the update case, as well as deletion as well as dumpAllConfig and replaceAllConfig operations.
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class V0_35_28_001__AddActorCatalogMetadataColumns extends BaseJavaMigration { |
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 add a test for the migration?
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.
Done!
83a2af4
to
7e2b69c
Compare
Both ActorCatalog and ActorCatalogFetchEvent are now updated during a writeConfig operation if their UUID is already present in DB.
- delete - replaceAllConfigs - dumpConfigs
d88fd4e
to
eb0a589
Compare
What
This adds persistence functions for the schema tables added in #10286
This PR is part of #9895