[infinispan-dev] Refactoring async operations

[Dan Berindei]

On Thu, Nov 22, 2012 at 3:31 PM, Mircea Markus <mmarkus at redhat.com> wrote:

>
> On 22 Nov 2012, at 10:16, Dan Berindei wrote:
>
>
> On Thu, Nov 22, 2012 at 11:53 AM, Galder Zamarreño <galder at redhat.com>wrote:
>
>>
>> On Nov 21, 2012, at 4:49 PM, Mircea Markus <mmarkus at redhat.com> wrote:
>>
>> > Hi,
>> >
>> > Part of fixing ISPN-2435, I need to significantly change
>> DistributionInterceptor which at the moment is a very complex pice of code.
>> Building the fix on top of it is extremely difficult and error prone, so I
>> need to refactor it a bit before moving forward.
>> > One such refactoring is about changing the way the async operations are
>> handled (e.g. putAsync()). At the moment all the interceptor calls happen
>> in user's thread, but two remote calls which are invoked with futures and
>> aggregated:
>> > the L1 invalidation and the actual distribution call. The code for
>> handling this future aggregation is rather complicated and spreads over
>> multiple classes (RpcManager, L1Manager, ReplicationInterceptor,
>> DistributionInterceptor), so the simple alternative solution I have in mind
>> is to build an asycPut on top of a syncPut and wrap it in a future:
>> >
>> > CacheImpl:putAsync(k,v) {
>> >     final InvocationContext ic =
>> createInvocatinonContextInCallerThread(); //this is for class loading
>> purpose
>> >     return asyncPoolExecutor.submit(new Callable() {
>> >          public Object call() {
>> >              return put(k,v, ic); //this is the actual sync put
>> >          }
>> >     }
>> > }
>> >
>> > This would significantly simplify several components ( no references to
>> network/aggregated futures in RpcManager, L1Manager,
>> ReplicationInterceptor, DistributionInterceptor).
>>
>> ^ At first glance, that's how I'd have implemented this feature, but
>> Manik went down the route of wrapping in futures only those operations that
>> went remote.
>>
>> Maybe he was worried about ctx switch cost? Or maybe about ownership of
>> locks when these are acquired in a separate thread from the actual caller
>> thread?
>>
>
> Speaking of locks, does putAsync make sense in a transactional context?
>
> Good point, I don't think async operation should work in the context of
> transaction: that would mean having two threads(the async operation thread
> and the 'main' thread) working on the same javax.transaction.Transaction
> object concurrently which is something not supported by most TM afaik, and
> something we don't support internally.
>
>
I'm not sure, but I think it is supported now - the only things happening
on a different thread only care about the cache's transaction, and not
about the TM transaction.


> There may be another backwards compatibility issue here, with listeners
> that expect to be called on the caller's thread (e.g. to use the TM
> transaction that's stored in a thread-local).
>
>
>> > Possible issues:
>> > - caller's class loader - the class loader is aggregated in the
>> InvocationContext, so as long as we build the class loader in caller's
>> thread we should be fine
>>
>> ^ To be precise, we don't build a class loaders. I guess you're refering
>> at building the invocation context.
>>
>> These days we're more tight wrt the classloader used, avoiding the
>> reliance on the TCCL, so I think we're in a safer position.
>>
>> > - IsMarshallableInterceptor is used with async marshalling, in order to
>> notify the user when objects added to the cache are not serializable. With
>> the approach I suggested, for async calls only (e.g. putAsync) this
>> notification would not happen in caller's thread, but async on
>> future.get(). I really don't expect users to rely on this functionality,
>> but something that would change never the less.
>>
>> ^ I don't think this is crucial. You need to call future.get() to find
>> out if things worked correctly or not, regardless of cause.
>>
>> > - anything else you can think of?
>> >
>> > I know this is a significant change at this stage in the project, so I
>> really tried to go without it - but that resulted in spaghetti code taking
>> a lot of time to patch. So instead of spending that time to code a complex
>> hack I'd rather go for the simple and nice solution and add more unit tests
>> to prove it works.
>>
>> ^ Have you done some experimenting already?
>>
>> Cheers,
>>
>> >
>> > Cheers,
>> > --
>> > Mircea Markus
>> > Infinispan lead (www.infinispan.org)
>> >
>> >
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20121122/737a92ff/attachment.html