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

Consider impact of Configuration interface on non-compliant implementations. #252

Closed
brianoliver opened this issue Oct 24, 2013 · 5 comments

Comments

@brianoliver
Copy link
Member

Currently all implementations that provide their own configurations are forced to implement all of the methods of the Configuration interface.

This implies that all implementations must provide configuration for: read-through, listeners, writers, loaders, expiry etc, regardless of whether they can provide said support.

eg: If an implementation simply wants to make use of the Cache interface (in a non-compliant manner) say for get/put etc operations, the implementation is forced to provide/return values for read-through, loaders, writers, expiry policy configurations, even if they don't make sense.

While it's very important the compliant implementations provide support for these features, it's equally important to allow for custom implementations (that may be non-compliant or may be configured differently yet still be compliant).

One solution would be to refactor the interface into separate parts, say for each feature "area", then provide a minimal part that all implementations (compliant and non-compliant) could support. This "minimal part" would only supply the type information and store-by-reference/value semantics for configuration. eg: Change the Configuration interface to look something like this.

interface Configuration<K, V> {
  /**
   * Determines the required type of keys for {@link Cache}s configured
   * with this {@link Configuration}.
   *
   * @return the key type or <code>Object.class</code> if the type is undefined
   */
  Class<K> getKeyType();

  /**
   * Determines the required type of values for {@link Cache}s configured
   * with this {@link Configuration}.
   *
   * @return the value type or <code>Object.class</code> if the type is undefined
   */
  Class<V> getValueType();

  /**
   * Whether storeByValue (true) or storeByReference (false).
   * When true, both keys and values are stored by value.
   * <p/>
   * When false, both keys and values are stored by reference.
   * Caches stored by reference are capable of mutation by any threads holding
   * the reference. The effects are:
   * <ul>
   * <li>if the key is mutated, then the key may not be retrievable or
   * removable</li>
   * <li>if the value is mutated, then all threads in the JVM can potentially
   * observe
   * those mutations,
   * subject to the normal Java Memory Model rules.</li>
   * </ul>
   * Storage by reference only applies to the local heap. If an entry is moved off
   * heap it will need to be transformed into a representation. Any mutations that
   * occur after transformation may not be reflected in the cache.
   * <p/>
   * When a cache is storeByValue, any mutation to the key or value does not
   * affect
   * the key of value stored in the cache.
   * <p/>
   * The default value is <code>true</code>.
   *
   * @return true if the cache is store by value
   */
  boolean isStoreByValue();
}

Once this is in place we could introduce a few other sub-interfaces to define the configuration of specific specialized features.

eg 1: an IntegrationConfiguration sub-interface that would contain all of the read-through, loader, writer configuration.

eg 2: a ManagementConfiguration sub-interface that would contain the management / statistics configuration.

eg 3: an ObservableConfiguration sub-interface that would contain the listener configuration.

Lastly, the MutableConfiguration class would actually remain the same, but simply implement all of these interfaces. All TCK tests and the RI would remain unchanged.

Importantly this would pave the way for allowing non-compliant but eventually compliant implementations for products like Memcached without us ever having to change the specification API. We'd simply choose to relax some of the TCK requirements - and only test certain configuration interface implementations.

This would also allow us to avoid introducing the concept of "profiles" or changing the API radically in the future.

The impact on existing implementations would be almost nil.

@gregrluck
Copy link
Member

Why can't a non-compliant implementation simply return Object.class for key and value, and boolean for the true false ones and null for the listeners etc.

And then on the MutableConfiguration side, thy can throw an UnsupportedMethodException when methods that they do not support a called.

This seems straight forward to me.

@brianoliver
Copy link
Member Author

The challenge is that the Configuration interface is used for/serves two purposes.

Purpose 1. Providing Configuration to an implementation.
Purpose 2. Returning Configuration information to an application (via a call to Cache#getConfiguration).

Purpose 1:
Should an implementation not require or not support all of the features specified by the Configuration interface, the said implementation is required by contract to provide all of the properties (like read-through, write-through, expiry, listeners etc).

Let's consider a hypothetical case for say Memcached that doesn't support read-through, write-through, expiry etc. Such a vendor is required by contract (of the Configuration interface) to provide implementations of all of the getters, even though they don't make sense.

public class MemcachedConfiguration<K, V> implements Configuration<K, V> {
   ... all of the Configuration methods (read-through, write-through et al) need to be implemented here, even though they don't make sense ...
}

What I'm saying is that the minimum interface for defining/configuring a cache could be, as you point out, as simple as key type, value type and whether it's by reference / by value. Everything else is mostly not required by an application wanting to do just simple caching.

Purpose 2:
As you point out, to solve the above issue it's relatively easy to throw UnsupportedOperationExceptions, but when you look at the resulting implementation, it turns into a mess - every method throws UnsupportedOperationException, which leads me to believe that an interface is required for said methods. ie: It's generally bad form for a Class to say it implements an interface and then throw UnsupportedOperationExceptions, especially everywhere.

It's also not clear for developers (looking at the Javadoc API say for the hypothetical MemcachedConfiguration, what is supported).

What's interesting is that we've gone to the trouble of cleanly isolating each of the features (like integration, events etc) into separate packages, but we've not done so at the Configuration level. I'm not saying we should relax the requirements for TCK compliants here, but if we follow the same pattern of separating out Configuration, into separate interfaces (like ObservableConfiguration, IntegrationConfiguration etc as an idea), it leads to a very clean API that allows both compliant and non-compliant implementations to clearly express what they support and don't necessarily support without ever having to throw an UnsupportedOperationException.

The impact on the RI and TCK would effectively be zero as the MutableConfiguration class would implement all of the interfaces and the SPEC would clearly say that all are mandatory (as they are now).

In the future, should we require it, we can relax that requirement without changing the API, to allow currently non-compliant implementations (like a hypothetic Memcached implementation) to be compliant. We'd simply change the wording in the SPEC document and update the TCK.

@gregrluck
Copy link
Member

After discussion, we can see the advantages, to encourage non TCK compliant API usage. It would work as follows:

  1. Have two configurations now, with possibly more in future versions: Configuration and CompleteConfiguration.
  2. getConfiguration signature changes to T getConfiguration(Class class);
  3. createCache becomes <K, V, C extends Configuration<K, V>> Cache<K, V> createCache(String cacheName, C configuration);

e.g. Cache<String, Integer> cache = cacheManager.createCache("my-cache", new MutableConfiguration<>());

  1. Cache usage is per the following examples. Note that this scheme supports returning proprietary configuration types, a nice advantage.

cache.getConfiguration(Configuration.class);
cache.getConfiguration(EHCacheConfiguration.class);
Configuration config = cache.getConfiguration(Configuration.class);
CompleteConfiguration config = cache.getConfiguration(CompleteConfiguration.class);
CoherenceCacheConfiguration config = cache.getConfiguration(CoherenceCacheConfiguration.class);

gregrluck added a commit that referenced this issue Nov 11, 2013
@gregrluck
Copy link
Member

Implemented.

Have some new tests and spec changes to do.

gregrluck added a commit that referenced this issue Nov 11, 2013
@gregrluck
Copy link
Member

Tests done and spec changed.

gregrluck added a commit that referenced this issue Nov 11, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants