My gut feeling is that to fix this properly there really should be 2 flags, one to indicate if the conditional operation succeeded or not, the 2nd to indicate whether the command should make a change in the container. The latter flag, based on the 1st flag, would decide which value to put in the container, the previous value (if conditional operation failed), or the new value (if conditional flag suceeded).

We already have two flags (isChanged and WriteCommand.isSuccessful), can't we use these?

@Mircea, that was precisely Pedro's fix but as I said, that causes other issues which are not as easy to fix because of the overloaded use of isChanged. This flag really means: "should the new value be applied to the container?", which based on the code must return true when:

1. A conditional operation succeeds.
2. When an entry in the cache store, should be stored in the cache as a result of an activation, or as result of loading it from the cache to check the result of the conditional operation, and the conditional operation fails to succeeded.

If 1 is true, the new value is updated in the cache. If 1. is false, the old value is loaded into the cache.

The problem that Pedro found is that the conditional operation did not succeed (point 1.), so according to him isChanged=false, but if you do that, the entry is not stored in the cache (point 2.) and other tests fail.

My gut feeling is that to fix this properly there really should be 2 flags, one to indicate if the conditional operation succeeded or not, the 2nd to indicate whether the command should make a change in the container. The latter flag, based on the 1st flag, would decide which value to put in the container, the previous value (if conditional operation failed), or the new value (if conditional flag suceeded).

The fix I submitted is really a stop gap, since separating isChanged into two flags would require further refactoring.

In the current code base, when a versioned entry fails to update due to conditional operation not fulfilling the condition, the previous value entry is added to the context with isChanged=true (if present in the cache, as a result of marking RepeatableReadEntry.copyForUpdate), and then EntryWrappingInterceptor loops through all the entries in the context, and if "changed", it will try commit them.

A better fix would be for the previous value to have its isChanged flag set to false rather than true. The flag being set to true is incorrect, as the entry is not actually changed and might cause problems in other scenarios as well. The value of the flag can be set in the conditional command perform() method.

Fix for 1 is not as simple as it looks:

• In the current code base, when a versioned entry fails to update due to conditional operation not fulfilling the condition, the previous value entry is added to the context with isChanged=true (if present in the cache, as a result of marking RepeatableReadEntry.copyForUpdate), and then EntryWrappingInterceptor loops through all the entries in the context, and if "changed", it will try commit them. What happens here is that the entry is not the result of an actual update in the transaction, so its version is null and it breaks later on when ClusteredRepeatableReadEntry checks for the version.

• Pedro's fix works for this particular case, but breaks other use cases, such as CacheLoaderFunctionalTest.testLoading and PassivationFunctionalTest.testRemoveAndReplace. What happens there is that if an entry is in the cache but not in the cache, and the conditional operation to try to putIfAbsent a new value fails, it should load the previous value into the cache. With Pedro's fix, the cache is no longer updated since it's marked that entry to not be "changed", and it's "changed" since you're storing it in the cache, even if it's the result of activating/loading the entry.

A better fix IMO would be to avoid committing the entry if the version is null in the 1st place. This is a more controlled or isolated fix, and avoids messing up with the business of unsetting the "changed" flag which is a bit fragile. Having just tested this quickly, it breaks other things... I'll keep trying things...

Adrian, wrt problem 3, did you create a separate issue for it in the end? If so, what's the JIRA?

Problem 2

Adrian, thx for taking 2 and 3. Next time make sure you add 'links' to JIRAs to establish relationships between them. It makes it easier to track jiras.

I can confirm now that item 2 was solved as ISPN-2530.

I have also analysed item 3 and will extract it as a separate issue because I need it for ISPN-2362.

Galder, if you are looking at this issue please take care only of item 1. Thanks!

It's very probable that item 2 is identical to ISPN-2530.