Task: Review trivago's findings

[cruftex]

@christian-esken has some findings for the Spec which look quite relevant:
https://github.com/trivago/triava/blob/master/tck/README.md

This is a task for me to look it through and open specific issues for it and give back feedback.

[cruftex]

Current state of the readme I am going to address:

PutTest.putAll_NullKey()

// It disallows partial success, even though this is not explicitly required, instead the Spec reads:
//
// " * The effect of this call is equivalent to that of calling
//   * {@link #put(Object, Object) put(k, v)} on this cache once for each mapping
//   * from key <tt>k</tt> to value <tt>v</tt> in the specified map.
//   *
// * The order in which the individual puts occur is undefined."
//
// There is no mention of a "rollback" for partial failures. In contrast CacheWriter has explicit documentation ("remove succesful writes from the Map").
// The API / Spec should be changed to reflect the TCK test or vice versa.

CacheManagerTest createCacheSameName() and createCacheSame()

// Observation: Mandates to throw CacheException when "the same" cache is to be created twice.
// Issue: Javadocs has no "@throws CacheException". It is only in the text. 

CacheManagerTest getNullTypeCacheRequest()

// Observation: Mandates to throw NullPointerException when passing null as keyClass or valueClass
// Issue: Javadocs and Spec do not mention behavior on null.

org.jsr107.tck.event.CacheListenerTest.testFilteredListener(CacheListenerTest.java:396)

// Observation: Unclear spec for Cache.remove(key): We need to use oldValue as value in the event. The Spec is not clear about this, but the TCK bombs
// us with NPE when we would use null as "value" and oldValue as "old value". The JSR107 RI uses oldValue as "value" and leaves the "old value" unset.
// org.jsr107.tck.event.CacheListenerTest$MyCacheEntryEventFilter.evaluate(CacheListenerTest.java:344)  // <<< Location of NPE

CacheMBStatisticsBeanTest

// Three wrong assertEquals() checks, where the "expected" and "actual" parameters are exchanged.
// Has no influence on test result, but wrong test output.
    assertEquals(result, "Sooty");
    assertEquals(result, "Trinity");

org.jsr107.tck.processor.CacheInvokeTest.noValueException()

// Observation: This test checks for IllegalAccessError, but the ThrowExceptionEntryProcessor class wraps it and throws "new EntryProcessorException(t);"
// An implementation should wrap EntryProcessor Exceptions also in EntryProcessorException, which means the IllegalAccessError gets double wrapped.

// Issue: The noValueException() test treats double wrapping as wrong, but IMO the Spec says to wrap ALL EntryProcessor Exceptions.

// Proposed solution: Change the TCK, or change the Spec to explicitly say that "exc instanceof EntryProcessorException" should not be wrapped again.  
// See: https://github.com/jsr107/jsr107tck/issues/85

org.jsr107.tck.processor.CacheInvokeTest

// Observation: nullProcessor() and invokeAll_nullProcessor() require a NullPointerException.

// Issue: It is not mentioned in the Spec. An alternative would be to ignore a null processor. 

// Proposed solution: Add "@throws NullPointerException if entryProcessor is null" to Javadocs

org.jsr107.tck.integration.CacheLoaderTest

// Observation: shouldPropagateExceptionUsingLoadAll() requires a CacheLoaderException

// Issue: Javadocs do not mandate this for Cache.loadAll().
//    a) Javadoc states: "If a problem is
//     encountered during the retrieving or loading of the objects,
//     an exception is provided to the {@link CompletionListener}."
//     b) The @throws declaration is "@throws CacheException        thrown if there is a problem performing the load."

// Proposed change: Change CacheLoaderTest to check for CacheException
// See: https://github.com/jsr107/jsr107tck/issues/99   

org.jsr107.tck.integration.CacheWriterTest

// Observation: SImilar to CacheLoaderTest
// CacheWriterException

org.jsr107.tck.integration.CacheLoaderWithoutReadThroughTest

• Observation: shouldLoadWhenAccessingWithEntryProcessor() requires that no load takes place for cache.invoke() in case of no-read-through

• Issue: While the Spec has a nice table about read-through and is clear about it, the JavaDocs do not mention it:

  // "If an {@link Entry} does not exist for the specified key, an attempt is made to load it (if a loader is configured)" (no mention of read-through)

• Proposed change: invoke() has fairly complex behaviour, and the documentation should be as clear as possible.
  // Old: "an attempt is made to load it (if a loader is configured)"
  // New: "an attempt is made to load it (if the cache is read-through)"

Unclear spec which kind of Listener should fire on Evictions

// Observation: Spec is unlcear. It talks about "evictions" when doing "expiration", but not about "true" evicitions.
// ehcache sends EVICTED, it seems. I will go for it, but it should be clarified.

// Proposed change: Clarification

Spec: CacheWriter table

//  V getAndReplace(K key, V value)
// Observation: Wrong description "Yes, if this method returns true"

// Proposed change: "Yes, if this method returns a non-null value"

org.jst107.tck.??? <<< Determine the test(s) that fail

// Observation: javax.cache.Cache.Entry and MutableEntry need to be Serializable. Otherwise TCK fails (TODO: Add the test names)

// Proposed action: Clarify. Possibly add "Serializable" requirement to Spec

org.jsr107.tck.RemoveTest

• Observation: shouldWriteThroughRemove_SpecificEntry() mandates that remove(key,value) only(!) writes-through, if we happen to have the
  (key,value) combination in the local cache.
• Issue: This can lead to non-deterministic behavior if the local cache has evicted
  that Cache entry. It is also inconsistent with all other methods: Usually the write-through is always done, and the local
  Cache get mutated for the successfully written-through entries. But here the local Cache is inspected first.
• Proposed change: Unclear. The reason could be an omission in the CacheWriter Interface: It does not have a write(key,value) method. To be discussed.

[cruftex]

PutTest.putAll_NullKey()

Yes. TCK change: jsr107/jsr107tck#103
I also found: jsr107/jsr107tck#104

CacheManagerTest createCacheSameName() and createCacheSame()

No. Looks okay to me. CacheManager.createCache has proper throws CacheException

CacheManagerTest getNullTypeCacheRequest()

Yes. Tiny Spec clarfication in the JavaDocs.

#361
I also found: jsr107/jsr107tck#104

org.jsr107.tck.event.CacheListenerTest.testFilteredListener(CacheListenerTest.java:396)

Yes. Old value is only relevant to updates. It should be clarified. #362
Found also a System.out in the test. Cleanup: jsr107/jsr107tck#105

CacheMBStatisticsBeanTest

Yes. Cleanup: jsr107/jsr107tck#105

org.jsr107.tck.processor.CacheInvokeTest.noValueException()

Yes. Already covered by: jsr107/jsr107tck#85

org.jsr107.tck.processor.CacheInvokeTest

No. It is there: @throws NullPointerException if key or {@link EntryProcessor} is null

org.jsr107.tck.integration.CacheLoaderTest

Yes. Wrapper exceptions need to be checked for consistency. #346

org.jsr107.tck.integration.CacheLoaderWithoutReadThroughTest

Yes. #343

Unclear spec which kind of Listener should fire on Evictions

Eviction listeners are out of the scope of the Spec. Can you please address the areas that should be clarified?

Spec: CacheWriter table

Yes. #363

Observation: javax.cache.Cache.Entry and MutableEntry need to be Serializable

No. I am sure it is possible to pass 100% without beeing this serializable.

org.jsr107.tck.RemoveTest

No. The Spec defenition and TCK is consistent. Whether the defined behavior makes sense is another question. I think the behavior makes sense in the meaning of defining a consistent behavior, which a standard needs to do for everything, but does not necessarily provide a useful functionality. Maybe a cleaner way would be not to define any Writer effects at all, since this method is primarily meant to operate on the cache. See also the putIfAbsent() discussion at #303. JSR107 is in maitenance. We cannot change this behavior anyways.

[cruftex]

@christian-esken Completed the triage. I am only unsure about the eviction listeners. If you have any concrete findings where there are misleading formulations, let me know.

[christian-esken]

About the eviction listeners: You are right when saying "Eviction listeners are out of the scope of the Spec.". Yes, and exactly that is the problem. The unfortunate impact for applications is that entries can disappear from the Cache with no chance of the application being notified. So it is a lucky chance:

• If the entry expired: Hurray.
• If the entry is evicted: Bad luck.

For real-world scenarios such non-deterministic behavior is often not acceptable. If an application wants to know when an entry disappears from the Cache, it often wants to know that independent of the reason. This is why ehcache sends EVICTED in that case (if I read its source code correctly).

[cruftex]

Correct :)

There is another perspective, too:

• You cannot configure when the cache is evicting at all
• The Spec defines expiry and expiry listeners, but does test anything that is async or has some time delay => Its good that we have expiry listeners specified, but they may or may not work as specified....