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

[Radim Vansa]

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.

*: 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

-- 
Radim Vansa <rvansa at redhat.com>
JBoss DataGrid QA