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

What is the caching specification for Config? #370

Closed
ljnelson opened this issue Jun 8, 2018 · 11 comments
Closed

What is the caching specification for Config? #370

ljnelson opened this issue Jun 8, 2018 · 11 comments
Assignees
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Milestone

Comments

@ljnelson
Copy link
Contributor

ljnelson commented Jun 8, 2018

The relevant section of the specification for MicroProfile Config says only this:

A Config instance provides no caching but iterates over all ConfigSources for each getValue(String) operation. A ConfigSource is allowed to cache the underlying values itself.

This speaks to the getValue(String, Class) method of Config, but does not mention the getOptionalValue(String, Class) or getPropertyNames() (or getConfigSources()) methods.

The phrase "A Config instance provides no caching" make It sound like perhaps the intent of the caching specification of Config is that, loosely speaking, no caching at all must occur in any implementations of any of these methods. That is, getConfigSources() should always check to see if ConfigSources have been added or removed or otherwise changed since the last invocation, and getPropertyNames() should aggregate invocations of each such ConfigSource's getPropertyNames() method, each time, and getOptionalValue(String, Class) should use the same algorithm described by the specification for getValue(String, Class).

Is this a correct interpretation of the intent of the specification?

@struberg
Copy link
Contributor

struberg commented Jun 8, 2018

Hi Laird!

All the caching is born from practical needs. At a customer we have about 20k requests/min and server and we read tons of config values with each request. If we would go e.g. to the properties Table in the DB for each Config#getValue().... You get the idea.

There are 3 parts involved in this story:
1.) the ConfigSources. They might use some caching to speed up access
2.) Config#getValue() this part does NOT cache. It always goes through to the ConfigSources!
3.) ConfigAccessor (new in ConfigJSR, will be backported to mp-config soon): This contains a user specified cache strategy! Including eager cache invalidation in case a ConfigSource actively notifies us that something got changed.

The are many subtle details involved and we went through long discussions and many years of experience in this field. Might be the easiest if you could join the mp-config hangout next time.

@ljnelson
Copy link
Contributor Author

ljnelson commented Jun 8, 2018

Thanks, Mark; hangouts are fine and good but I'm looking for what's in the actual specification. Thanks for your reply.

From your reply I understand that at least in version 1.2.1:

  • a ConfigSource implementation might or might not use caching in its methods; the specification does not say one way or the other
  • a Config#getValue(String) implementation must never cache

What I want to find out in version 1.2.1 is:

  • Must a Config#getPropertyNames() implementation cache? Or must it never cache? Or is it undefined?
  • Must a Config#getOptionalValue(String) implementation cache? Or must it never cache? Or is it undefined?
  • Must a Config#getConfigSources() implementation cache? Or must it never cache? Or is it undefined?

@ljnelson
Copy link
Contributor Author

ljnelson commented Jun 8, 2018

So that I'm not just complaining 😄 here are some example Javadocs in the style of what I would expect to see in anything that calls itself a specification, and not just a sketch or a hint.

Here are some Javadocs I'd expect to see on the Config#getPropertyNames() method if caching is required, i.e. if caching is part of its specification:

 /**
  * Returns a non-{@code null}, {@linkplain
  * Collections#unmodifiableSet(Set) unmodifiable} and
  * unchanging {@link Set} of {@link String}s representing the
  * names of properties whose values may be retrieved with
  * the {@link #getValue(String, Class)} method.
  *
  * <p>Implementations of this method must invoke the {@link
  * ConfigSource#getPropertyNames()} method on each of their
  * {@linkplain #getConfigSources() constituent
  * <code>ConfigSource</code>s} and must combine their
  * results, discarding duplicate names.  The result of this
  * combination must be cached indefinitely.</p>
  *
  * @return a non-{@code null}, unchanging, {@linkplain
  * Collections#unmodifiableSet(Set) unmodifiable
  * <code>Set</code>} of cached property names resulting from
  * the aggregation of the property names retreivable from this
  * {@link Config} implementation's {@linkplain
  * #getConfigSources() constituent
  * <code>ConfigSource</code>s}
  */
  public Set<String> getPropertyNames();

Or, if caching is forbidden, i.e. the absence of caching is part of the specification, I'd expect to see this instead:

 /**
  * Returns a non-{@code null}, {@linkplain
  * Collections#unmodifiableSet(Set) unmodifiable} and
  * unchanging {@link Set} of {@link String}s
  * representing the names of properties whose values may
  * be retrieved with the {@link #getValue(String, Class)} method.
  *
  * <p>Implementations of this method must ensure that with
  * every invocation of this method, the {@link
  * ConfigSource#getPropertyNames()} method on each of their
  * {@linkplain #getConfigSources() constituent
  * <code>ConfigSource</code>s} is invoked and its results are
  * combined with all others, discarding duplicate names.
  * <strong>The result of this combination must never be
  * cached.</strong></p>
  *
  * @return a non-{@code null}, unchanging, {@linkplain
  * Collections#unmodifiableSet(Set) unmodifiable
  * <code>Set</code>} of property names resulting from
  * the aggregation of the property names retreivable from this
  * {@link Config} implementation's {@linkplain
  * #getConfigSources() constituent
  * <code>ConfigSource</code>s}
  */
  public Set<String> getPropertyNames();

I might also add something like this:

  * <p>Implementations of this method must ensure that the
  * {@code Set} returned may be used without external synchronization.</p>

…if it is true. (The specification says that all methods in Config implementations must be thread-safe, but it does not say anything about the return values from those methods.)

With language such as this, I would know definitively how to implement this method. Without language such as this I would not know definitively how to implement this method.

@struberg
Copy link
Contributor

struberg commented Jun 8, 2018

One step after the other:

Must a Config#getPropertyNames() implementation cache? Or must it never cache? Or is it undefined?

Intentionally undefined. Note that this method is usually not used that often. So in my impl I do not have caches but always go through to the ConfigSources. But you could do it different.

Must a Config#getOptionalValue(String) implementation cache? Or must it never cache? Or is it undefined?

Same behaviour as with getValue()!

Must a Config#getConfigSources() implementation cache? Or must it never cache? Or is it undefined?

getConfigSources() returns the ConfigSources registered or detected at startup. And the Config 'infrastructure' does not change afterwards (Config infra is basically immutable). If you consider this as 'caching' then yes.

@ljnelson
Copy link
Contributor Author

ljnelson commented Jun 8, 2018

Thank you; should this information go in the specification?

@struberg
Copy link
Contributor

struberg commented Jun 8, 2018

We could, but it would blow up the amount of things to specify. And this would be counter productive regarding readability. We could probably have some implementers notes. Also not sure how to TCK this.
The third bullet should be in the spec already.

We should imo be very careful to not over-specify it.

@ljnelson
Copy link
Contributor Author

ljnelson commented Jun 8, 2018

OK; could we add language that indicates that implementors are free to implement this method however they like?

(Also, you mention:

The third bullet should be in the spec already.

I didn't find a reference in the specification for this (the third bullet is "Must a Config#getConfigSources() implementation cache? Or must it never cache? Or is it undefined?"). If it's in there, I must have overlooked it; could you kindly point me to it?)

@Emily-Jiang
Copy link
Member

@struberg and I talked this in today's hangout. It is implementation details whether to cache it. If you cache it, you will have performance gain but latency. The spec does not force implementations to cache it or not. You may choose not to cache it.

@ljnelson
Copy link
Contributor Author

Related: #371, #395, #431

@jmesnil jmesnil added clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API mpconfig-1.4 labels Aug 2, 2019
@Emily-Jiang Emily-Jiang added this to the MP Config 1.4 milestone Oct 11, 2019
@Emily-Jiang
Copy link
Member

At today's hangout, we agreed that we are going to add a couple of sentences in the spec to state:
1.) the ConfigSources. They might use some caching to speed up access
2.) Config#getValue() this part does NOT cache.
as per #370 (comment)

@Emily-Jiang Emily-Jiang self-assigned this Nov 22, 2019
@Emily-Jiang
Copy link
Member

I am trying to add something as noted above. However, I double checked the spec. It has the following already:

A Config instance provides no caching but iterates over all ConfigSources for each getValue(String) operation. A ConfigSource is allowed to cache the underlying values itself.

With is, I am closing this as done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clarification 🏆 A clarification (PR) or request for clarification (issue) in the spec or API
Projects
None yet
Development

No branches or pull requests

4 participants