Skip to content
This repository was archived by the owner on Apr 15, 2024. It is now read-only.

Conversation

@mcriscolo-cs
Copy link
Contributor

@mcriscolo-cs mcriscolo-cs commented Jul 26, 2023

Due to this issue: https://youtrack.jetbrains.com/issue/KT-41373 kotlinx serialization's generated serializer is not behaving well with Jackson JSON. This killed the catalog management example. Reimagined the example to use an instance of LocalCatalog and a spec directory with resources defined. These resources are deserialized and then loaded into the CortexSession catalog (local or remote) as it did in the original example

@mcriscolo-cs mcriscolo-cs requested a review from laguirre-cs July 27, 2023 19:56
laguirre-cs
laguirre-cs previously approved these changes Jul 28, 2023
Copy link
Collaborator

@laguirre-cs laguirre-cs left a comment

Choose a reason for hiding this comment

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

Looks fine to me, just one minor comment.

Comment on lines +219 to +221
LocalCatalog localCatalog;
try {
localCatalog = new LocalCatalog(specPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it not the same thing if you use the Catalog implementation from the CortexSession ? Catalog localCatalog = cortexSession.catalog()? If it is, then it might be preferable since the entrypoint for the user/developer is still the CortexSession

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is actually a separate instance of the LocalCatalog, the instance will act as a source. The example is all about updating/maintaining the resource entities before doing some work on them. The catalog in the CortexSession is a singleton and is our target catalog (may be pointing at a local or remote catalog)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants