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

[Dan Berindei]

On Tue, Oct 29, 2013 at 1:33 PM, Pedro Ruivo <pedro at infinispan.org> wrote:

>
>
> 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)?
>

I don't think we actually fetch the value before the write operation for
non-tx caches, at most we do an extra lookup in the local data container
(in NonTxDistributionInterceptor.remoteGetBeforeWrite).

We do fetch the value explicitly in transactional caches, but we don't
invoke the write command remotely in that case.

There is an opportunity for optimization in pessimistic tx caches, because
we invoke both a remote get and a remote lock command for the same write
command. Perhaps we can make LockControlCommand return the previous value
(only if needed, of course).



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

+1 for ConditionalWriteCommand



>  >
> > My 2c.
> >
> > Radim
> >
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20131029/ac03e961/attachment-0001.html