[infinispan-dev] Improve WriteCommand processing code and [possibly] performance

[Pedro Ruivo]

On 10/29/2013 07:58 AM, Radim Vansa wrote:
> On 10/25/2013 08:17 PM, Pedro Ruivo wrote:
>> Hi guys.
>>
>> I've open a JIRA to tack this: https://issues.jboss.org/browse/ISPN-3664
>>
>> Suggestions/feedback is appreciated. This is probably be integrated in
>> the next major (but no promises).
>>
>> I was not cleared just ping me.
>>
>> Have a nice weekend folks :)
>>
>> Regards,
>> Pedro
>>
>> Description is the following:
>>
>> Major refactorization of the write command with the following goals:
>>
>> * Base WriteCommand: all the write command has the same workflow through
>> the interceptor chain
>> * Create a concrete WriteCommand for each operation (put, remove,
>> replace, replaceIfEquals, removeIfEquals, putIfAbsent)
>> * Extend the interceptor chain to process each one of the command and
>> add a new "visitWriteCommand", that is invoked by the default visitX
>> methods.
>> * (minor) change the GetKeyValueCommand to ReadCommand to make name
>> "compatible" with WriteCommand.
>>
>> Note that most of the interceptor only needs to implement the
>> visitWriteCommand because all the write command has the same execution
>> flow. The basic flow of the write commands are: (non-tx) lock, fetch
>> value (cachestore/remote), check condition and apply change. for tx
>> mode, lock (if pessimistic), fetch value (cache loader, remote, etc),
>> apply change and add it to the transaction (if successful)
> I've been wondering for a while why the fetch part is a separate message
> in the write flow. Is the return value of much use when it does not
> return really the replaced value but only some of the previous values?
> And this way you double* the latency. I think that returning the value
> from primary owner as the response for write would make more sense.
> Naturally, for optimistic txs you can only do the get, and for
> pessimistic ones you'd have to return the value together with the
> locking, but still, the operations would make more sense.

Actually, I think you are right. It makes no sense to fecth the value 
before the write operation for non-tx caches. I also like your idea of 
fetching the value when the key is locked :)

Do you mind to create a new thread for it (I'm not sure if everybody 
reads this thread)?

>
> *: OK, it's not doubling as with the write the primary owner replicates
> the write to backups, but it's doubling the amount of communication
> originating from the requestor node.
>
>>
>> Also, another advantage is the simplification of the EntryFactory
>> because if we think a little about it, independent of the write command
>> we need to wrap the entry anyway.
>>
>> Suggested implementation
>>
>> class abstract WriteCommand,
>> Object key, Object newValue
>> boolen match(Object currentValue) //true by default
>> boolean needsRemoteGetBeforeWrite() //true by default
>> object perform() //common implementation like: if
>> (match(entry.getValue()) then entry.setValue(newValue);
>> entry.setChanged(true); entry.setRemoved(newValue == null)}
>>
>> * Concrete implementations *
>>
>> {PutCommand|RemoveCommand} extends WriteCommand
>> ovewrite needsRemoteGetBeforeWrite() {return
>> !flags.contains(IGNORE_RETURN_VALUE)}
>>
>> ReplaceIfPresentCommand extends WriteCommand
>> ovewrite match(Object currentValue) {return currentValue != null}
>>
>> PutIfAbsentCommand extends WriteCommand
>> ovewrite match(Object currentValue) {return currentValue == null}
>>
>> * Special base class for operation with expected value to compare *
>>
>> class abstract AdvancedWriteCommand extends WriteCommand
>> Object expectedValue
>> match(Object currentValue) {return currentValue.equals(expectedValue)}
> I'd call that rather ConditionalWriteCommand - AdvancedWC sounds too
> general to me.
>
> My 2c.
>
> Radim
>