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.

Sign up for GitHub

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

Jump to bottom

Serde does not cache schemas when not given a version #3824

Closed
gr8routdoors opened this issue Oct 16, 2023 · 1 comment
Closed

Serde does not cache schemas when not given a version #3824

gr8routdoors opened this issue Oct 16, 2023 · 1 comment
Assignees
@carlesarnal
Labels
area/storage Bug Something isn't working port-3.x Port from 2.x to 3.x

Comments

@gr8routdoors
Copy link
Contributor

Description

Registry
Version: 2.4.x, 2.5.x
Persistence type: All

When looking up an artifact using Group and Artifact ID (e.g. by reference) without a version, the serde (via DefaultSchemaResolver) will lookup and use the latest version of a given artifact. However, when it goes to cache the schema, it caches it with the version number. This is fine for future lookups that include version, but will never succeed for future lookups that do not include a version, as the version is always null in these types of lookups.

It can be debated as to whether this type of behavior is desirable, as some may never want to cache latest lookups. However, I would argue that caching should be the default, as it came as quite a surprise finding this at runtime, given all the settings that were presented for caching out of the box. Further, use of latest is quite convenient in some environments (such as a default CloudEvents setup). Not having serde caching by default puts a large amount of stress on Apicurio.

Environment

Affects all enviroments

Steps to Reproduce

  1. Configure the serde with a cache TTL long enough to easily perform a couple lookups without the cache expiring.
  2. Lookup an artifact through the serde, specifying just an Artifact ID and Group ID (the default group works fine)
  3. Lookup the same artifact again immediately afterwards
  4. You'll notice the Apicurio API was hit for both lookups

Expected vs Actual Behaviour

As mentioned above, I would expect both a key with the version and one without to be cached so that both lookups would work by default.

If this behavior is intended, then there should be an option to cache latest entries.

Additional Context

I'm happy to implement this functionality myself today, in the form of a configurable option. I just need to know whether I should make my change the default or not. Also, I would like to build on top of my #3823 so that I do not conflict.
@gr8routdoors gr8routdoors added the Bug Something isn't working label Oct 16, 2023
@gr8routdoors
Copy link
Contributor Author

FYI @carlesarnal, I discovered this as part of my work on #3823 . I'm going to address this today.

gr8routdoors pushed a commit to gr8routdoors/apicurio-registry that referenced this issue Oct 17, 2023
fix(schema-resolver): caching of latest artifacts Apicurio#3834
ce5dfda 
Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup.  This code changes the default behavior to index both the artifact with its version and the latest/null version.  It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired.  See Apicurio#3824 for details.
gr8routdoors pushed a commit to gr8routdoors/apicurio-registry that referenced this issue Oct 17, 2023
fix(schema-resolver): caching of latest artifacts Apicurio#3834
194406c 
Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup.  This code changes the default behavior to index both the artifact with its version and the latest/null version.  It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired.  See Apicurio#3824 for details.
@gr8routdoors gr8routdoors mentioned this issue Oct 17, 2023
Merged
gr8routdoors pushed a commit to gr8routdoors/apicurio-registry that referenced this issue Oct 17, 2023
fix(schema-resolver): caching of latest artifacts Apicurio#3834
e919428 
Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup.  This code changes the default behavior to index both the artifact with its version and the latest/null version.  It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired.  See Apicurio#3824 for details.
@gr8routdoors gr8routdoors mentioned this issue Oct 17, 2023
Merged
carlesarnal pushed a commit that referenced this issue Oct 20, 2023
@gr8routdoors
fix(schema-resolver): caching of latest artifacts and resilient schem…
742c4eb 
…a updates (#3839)

* feat(schema-cache): ERCache.configureFaultTolerantRefresh #3807 (#3823)

Adds the notion of a fault tolerance in refresh for production environments where it's better to use a stale cache value than die when a cache entry refresh fails.  See issue #3807 for details.

Co-authored-by: Devon Berry <devon.berry@riotgames.com>

* fix(schema-resolver): caching of latest artifacts #3834

Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup.  This code changes the default behavior to index both the artifact with its version and the latest/null version.  It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired.  See #3824 for details.

---------

Co-authored-by: Devon Berry <devon.berry@riotgames.com>
carlesarnal pushed a commit that referenced this issue Oct 23, 2023
@gr8routdoors
fix(schema-resolver): caching of latest artifacts #3834 (#3826)
f1e1569 
Caching of artifacts with no version (e.g. latest) did not work because reindex would use the new key that has the artifact version that was found in the lookup.  This code changes the default behavior to index both the artifact with its version and the latest/null version.  It also exposes a configuration property (`apicurio.registry.cache-latest`) where this behavior can be disabled for use cases where caching of latest is not desired.  See #3824 for details.

Co-authored-by: Devon Berry <devon.berry@riotgames.com>
@carlesarnal carlesarnal added the port-3.x Port from 2.x to 3.x label Oct 30, 2023
@carlesarnal carlesarnal self-assigned this Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@carlesarnal carlesarnal

Labels
area/storage Bug Something isn't working port-3.x Port from 2.x to 3.x
Projects
Registry 2.5
Status: Released
Milestone
No milestone
Development

No branches or pull requests
2 participants
@gr8routdoors @carlesarnal