On 22 Nov 2012, at 10:16, Dan Berindei wrote:
>
> On Thu, Nov 22, 2012 at 11:53 AM, Galder Zamarreño <gal...@redhat.com> wrote:
>
> On Nov 21, 2012, at 4:49 PM, Mircea Markus <mmar...@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.
>
> 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@lists.jboss.org
> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
>
> --
> Galder Zamarreño
> gal...@redhat.com
> twitter.com/galderz
>
> Project Lead, Escalante
> http://escalante.io
>
> Engineer, Infinispan
> http://infinispan.org
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev@lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
Cheers,
--
Mircea Markus
Infinispan lead (www.infinispan.org)
_______________________________________________
infinispan-dev mailing list
infinispan-dev@lists.jboss.org
https://lists.jboss.org/mailman/listinfo/infinispan-dev
