[infinispan-dev] Refactoring async operations

[Dan Berindei]

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?

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)
> >
> >
> >
> >
> > _______________________________________________
> > infinispan-dev mailing list
> > infinispan-dev at lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Galder Zamarreño
> galder at redhat.com
> twitter.com/galderz
>
> Project Lead, Escalante
> http://escalante.io
>
> Engineer, Infinispan
> http://infinispan.org
>
>
> _______________________________________________
> 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/20121122/4874a148/attachment.html