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.

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

Add persistence function for discovered schema #10326

Merged
merged 10 commits into from Feb 17, 2022
Merged

Conversation

malikdiarra
Copy link
Contributor

What

This adds persistence functions for the schema tables added in #10286

This PR is part of #9895

@CLAassistant
Copy link

CLAassistant commented Feb 14, 2022

CLA assistant check
All committers have signed the CLA.

@malikdiarra malikdiarra temporarily deployed to more-secrets February 14, 2022 21:54 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 14, 2022 21:54 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 00:17 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 00:17 Inactive
Copy link
Contributor

@tuliren tuliren left a 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?

malikdiarra and others added 4 commits February 14, 2022 18:58
…sistence/DatabaseConfigPersistence.java

Co-authored-by: LiRen Tu <tuliren.git@outlook.com>
…sistence/DatabaseConfigPersistence.java

Co-authored-by: LiRen Tu <tuliren.git@outlook.com>
…grations/V0_35_28_001__AddActorCatalogMetadataColumns.java

Co-authored-by: LiRen Tu <tuliren.git@outlook.com>
…grations/V0_35_28_001__AddActorCatalogMetadataColumns.java

Co-authored-by: LiRen Tu <tuliren.git@outlook.com>
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 03:00 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 03:02 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 03:19 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 03:20 Inactive
@malikdiarra
Copy link
Contributor Author

Looks good in general. Could you add more unit tests for the list- methods in DatabaseConfigPersistence?

This is done!

.from(ACTOR_CATALOG_FETCH_EVENT)
.where(ACTOR_CATALOG_FETCH_EVENT.ID.eq(actorCatalogFetchEvent.getId())));

if (!isExistingConfig) {
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 19:07 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 19:07 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 19:19 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 19:20 Inactive
Both ActorCatalog and ActorCatalogFetchEvent are now updated during a
writeConfig operation if their UUID is already present in DB.
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 20:04 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 20:04 Inactive
- delete
- replaceAllConfigs
- dumpConfigs
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 22:09 Inactive
@malikdiarra malikdiarra temporarily deployed to more-secrets February 15, 2022 22:09 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants