jsr107 / jsr107spec

JSR107 Cache Specification
Apache License 2.0
413 stars 164 forks source link

MutableEntry: improve JavaDoc and Spec clarification #343

Closed cruftex closed 7 years ago

cruftex commented 8 years ago

The actual JavaDoc of MutableEntry is not really capturing all the fundamentals.

Also check Spec for occurrences of "If loader defined" needs to be replaced with "If loader defined and read through configured"

There is also an interesting aspect, which is maybe missing in the Spec. E.g. the operation ...

CacheEntryProcessor<K, V, V> p = new CacheEntryProcessor<K, V, V>() {
  @Override
  public V process(MutableCacheEntry<K, V> entry, Object... arguments) throws Exception {
    if (!entry.exists()) {
      return null;
    }
    return entry.getValue();
  }
};
V cachedValue = invoke(key, p);

... would only return the value, if it is existing in the cache (some caches have getIfPresent for this).

But, what happens if the entry expires between the exists() and getValue()? IMHO the spec should say:

If exitsts() returned true, subsequent calls to getValue() always return a non null value from the cached entry and do not invoke the loader.

cruftex commented 8 years ago

Will prep an agreeable proposal.

gregrluck commented 7 years ago

Added Class level reference to EntryProcessor to explain the connection. Done.

Method getValue() should get overloaded and have some more description. It extends the contract of Entry and performs a load operation. Only invokes loader if read through is enabled. Done. Good suggestion. Makes things much clearer. Left out t

Also check Spec for occurrences of "If loader defined" needs to be replaced with "If loader defined and read through configured" No occurrences

These changes just address JavaDoc clarification. For the rest break those into separate issues in case we need to change behaviour. For those tests would need to be added.

cruftex commented 7 years ago

Commit was done in: https://github.com/jsr107/jsr107spec/commit/72373a0729446474a178cd7473287cb8ebf47537

For the rest break those into separate issues in case we need to change behaviour. For those tests would need to be added.

Hopefully the last point above is not changing behavior, but actually clarifying things that are implied by the atomic nature of the operation. But yes, its better to check/extend the TCK tests first.