Thanks for the feedback Galder! On 22 Nov 2012, at 09:53, Galder Zamarreño 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? > >> 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. yes, I'm referring to IC which aggregates the ClassLoader > > 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. +1 > >> - 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? Yes, I've pretty much implemented the code without this refactoring. But then ended up in a loop of fix regressions -> introduce new regressions, which was very hard to break because of the complexity of the code. > > 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 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